[ofa-general] [GIT PULL] please pull infiniband.git

Pradeep Satyanarayana pradeeps at linux.vnet.ibm.com
Mon Oct 29 11:03:45 PDT 2007


Roland Dreier wrote:
>  > Having waited for months for this patch to be merged in, it is very disappointing 
>  > to say the least. Wish it had been merged and if changes are needed they can always be 
>  > made subsequently. That has been my understanding of the development model.
> 
> If you really want to get into it...
> 
> I'll certainly accept some of the blame for taking too long to review
> this patch.  However, you didn't do yourself any favors by: a) making
> one huge ugly patch and b) being rather disagreeable when someone
> actually tried to review it.
> 
> As far as the development model goes, it is certainly true that for
> new things, we can merge first and fix later.  But when we're touching
> something like IPoIB, which is pretty critical to just about everyone
> using the IB stack at all, the standard is a little different: we need
> to be much more conservative.  And even for new stuff, starting from a
> good base is pretty important; it's easy to pick on coding style
> problems, and indeed they do make review harder, but it's even more
> important to have the underlying logic and structure be simple and
> maintainable.
> 
> Anyway, I'll post my current patch series shortly.  I think I was able
> to make the patch quite a bit neater and more reviewable: your patch
> added > 400 lines, while the main part of my series adds < 200 lines.
>

Roland, I realize a maintainers job is not easy with the incredible number of
patches that are submitted as I have seen on this mailing list. And I am glad 
to see that this patch is starting to see some forward movement at long last :). 
I will review the patch and test it out and provide comments. Thanks for your 
efforts and help with this.

There is some history behind why it became a huge ugly patch -which is not in
the patch itself, and why I resisted making changes (before the merge). In the 
initial stages when I submitted the patch it got a very chilly reception and the 
message I got was "hands off my code". If you will recollect I did raise 
maintainability issues in the very beginning. But, from the communications I 
inferred that without incorporating those comments I could not get the patch in. 
There were a few genuine issues that the reviews pointed out. At the same time a 
lot of inconsequential comments were made too. Over time the patch morphed to 
incorporate several such comments.

As you realize with a big patch, it is time consuming to test out every time the patch is
changed and that too across multiple HCAs. Even though I was in agreement with Sean's 
comments (I had proposed several of them early on :) ) I deferred making those changes because
they were undoing some of the changes suggested and I was not sure if there was agreement 
across the board. After all it had been reviewed by 3 people and continued to evolve. That 
is the reason I kept insisting that I would evaluate comments after the merge. Much easier 
to make small isolated changes after the big patch is in.

Anyway, I would like to move on and close the chapter on for-2.6.24 tree since the window is
now closed. Look forward to this patch being the first one to be merged into the for-2.6.25
tree.

Pradeep

Pradeep




More information about the general mailing list