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

[John Snow <jsnow@redhat.com>]

On 04/17/2015 12:01 PM, 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 <jsnow@redhat.com>
>> ---
>>   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...
>
> 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");
> 2517c2518,2519
> < /* New and old BlockDriverState structs for atomic group operations */
> ---
>  >
> /******************************************************************************/
>
>  > /* Transactions                                */
> 3439a3442,3443
>  >
>  >
> /******************************************************************************/
>
>
> 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
>

OK. I fixed the line length because checkpatch yelled at me.

I can add a micropatch that adds my organizational flair and fixes line 
lengths in a teensy patch that precedes this one.

Now: is the code movement necessary? well... Maybe, maybe not. I'd 
actually like to just break out transactions into their own file 
entirely, but that has its own challenges (like our heavy usage of 
internal block goodies) ...

My only real justification is in the cover letter: forward declarations 
of QMP functions is a burden.

Then again, so is basically erasing transaction development history...

:(