[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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.

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.