Re: [Qemu-block] [PATCH v2 08/11] block: move transactions beneath qmp interfaces

[Eric Blake]

On 04/17/2015 10:01 AM, Max Reitz wrote:
> On 27.03.2015 20:20, John Snow wrote:
>> In general, since transactions may reference QMP function helpers,
>> it would be nice for them to sit beneath them.
>>
>> This will avoid the need for forward declaring any QMP interfaces,
>> which would be aggravating to update in so many places.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   blockdev.c | 2581
>> ++++++++++++++++++++++++++++++------------------------------
>>   1 file changed, 1292 insertions(+), 1289 deletions(-)
> 
> I'm not so sure about this patch. I mean... 2581 changes? :-/
> 
> If you (or someone else) can convince me that forward declarations are
> really worse than this (is it more aggravating than having a patch with
> this diffcount?), well...

I like avoiding forward declarations of static functions, where it makes
sense, but I'm not going to insist on reordering code if it makes things
ugly.

> 
> But even then, I'd strongly vote for removing dropping all functional
> changes beside the code movement from this patch. Because I'm seeing this:
> 
> 2096c2096,2097
> <         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
> "power of 2");
> ---
>>         error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>>                   "granularity", "power of 2");

Indeed - you WANT code motion patches to be as easy as possible to
review.  From http://wiki.qemu.org/Contribute/SubmitAPatch

"Ideally, a code motion patch can be reviewed by doing git format-patch
--stdout -1 > patch; diff -u <(sed -n 's/^-//p' patch) <(sed -n
's/^\+//p' patch), to focus on the few changes that weren't wholesale
code motion."

> Probably sensible changes, but if you really, really, really want to go
> for this code movement, please apply them in an own patch before this
> one so that this one really is nothing but code movement.
> 
> Max
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature