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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@xxxxxxxxxxxxx]
> Sent: 10 March 2014 16:56
> To: Paul Durrant; George Dunlap
> Cc: George Dunlap; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v3 2/6] ioreq-server: tidy up use of ioreq_t
> 
> 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.
> 

The issue is that there were other places in the code that used "ioreq_t p[1]", 
so in patch 1 I elected to follow suit and then in patch 2 change all uses of 
that construct, including the one I'd introduced. Does that not seem logical? 
It seems logical to me. It creates a clear separation between code motion and a 
change of construct.
 
> I'd still rather they be in one patch, but that might be a taste thing.

I think so.

> 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.

I'm only introducing one occurrence of that construct. The majority were 
already present in the code before I touched it.

> 
> 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.
> 

It's possibly slightly clearer, but a re-base and re-test for the sake of that 
doesn't seem like a good ROI. These patches are not exactly the 'meat' of the 
series anyway :-)

  Paul

>   -George

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