Re: [Xen-devel] [PATCH v3 2/6] ioreq-server: tidy up use of ioreq_t

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 03/10/2014 04:04 PM, Paul Durrant wrote:
> > -----Original Message-----
> > From: dunlapg@xxxxxxxxx [mailto:dunlapg@xxxxxxxxx] On Behalf Of
> > George Dunlap
> > Sent: 10 March 2014 15:53
> > To: Paul Durrant
> > Cc: George Dunlap; xen-devel@xxxxxxxxxxxxx
> > Subject: Re: [Xen-devel] [PATCH v3 2/6] ioreq-server: tidy up use of ioreq_t
> >
> > On Mon, Mar 10, 2014 at 3:46 PM, Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > wrote:
> > > > -----Original Message-----
> > > > From: dunlapg@xxxxxxxxx [mailto:dunlapg@xxxxxxxxx] On Behalf Of
> > > > George Dunlap
> > > > Sent: 10 March 2014 15:43
> > > > To: Paul Durrant
> > > > Cc: xen-devel@xxxxxxxxxxxxx
> > > > Subject: Re: [Xen-devel] [PATCH v3 2/6] ioreq-server: tidy up use of
> > ioreq_t
> > > > On Wed, Mar 5, 2014 at 2:47 PM, Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > > wrote:
> > > > > This patch tidies up various occurences of single element ioreq_t
> > > > > arrays on the stack and improves coding style.
> > > > >
> > > > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > > Maybe I missed this in the earlier discussion, but why is most of this
> > > > not integrated into patch 1?
> > > It was a patch that was added after the v1 RFC patch series. I wanted to
> > keep it separate to avoid making patch 1 massively different to what it was
> > before.
> >
> > Isn't the point of the review process to change patches? :-)  In
> > general, both reviewers and code archaeologists (i.e., people going
> > through commits long after the fact) want as much as possible to know
> > what the end result is going to look like.  Having one patch which
> > does things one way, and another immediately following it that does
> > things a different way doesn't really help anybody, as far as I can
> > tell.  It only increases the amount of busy-work people have to do to
> > figure out what's going on.
> Patch 2 was added to code clean up that is not critical to this patch series. 
> IMO folding it into patch 1 obscures the original purpose of that patch. If 
> reviewers only cared about the end result then what's purpose of patch series 
> in the first place? May as well just fold everything into one patch.
So what you mean is, in the case of "ioreq_t p[1]" being replaced by 
"ioreq_t p", patch 1 puts it on the stack and patch 2 changes p from a 
pointer to a struct, and you think putting them in one patch makes it 
harder to discern one from the other.
I'd still rather they be in one patch, but that might be a taste thing.  
Another option might have been to have patch 1 do the code motion, and 
patch 2 use the ioreq on the stack instead -- then you're not 
introducing a weird construct that you're going to obliterate right 
away.  To do that, I guess you'd have to avoid making get_ioreq() static 
to hvm.c until patch 2.
But we're getting into bike-shed territory here.  I think it would be 
better to change the break-down, but I won't make that a condition for 
acceptance.
 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel