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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 03/10/2014 04:04 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of
>> George Dunlap
>> Sent: 10 March 2014 15:53
>> To: Paul Durrant
>> Cc: George Dunlap; xen-devel@lists.xen.org
>> 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@citrix.com>
>> wrote:
>>>> -----Original Message-----
>>>> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of
>>>> George Dunlap
>>>> Sent: 10 March 2014 15:43
>>>> To: Paul Durrant
>>>> Cc: xen-devel@lists.xen.org
>>>> 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@citrix.com>
>>>> 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@citrix.com>
>>>> 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