Re: [PATCH 1/3] block: add for_commit option to measure

[Jean-Louis Dupond <jean-louis@dupond.be>]

On 5/12/25 13:16, Hanna Czenczek wrote:
> On 16.04.25 09:16, Jean-Louis Dupond wrote:
>> To specify we use measure call for commit size calculations, we add a
>> new 'for_commit' option to the measure call.
>> This will be used in following commit to do a different measurement.
>
> Why not allow specifying the node name (or filename) of the commit 
> target instead of this just being a boolean?
Honestly didn't even thought about that option :)
>
> (That was my main problem with the original series, that it wasn’t 
> possible to specify the commit target (in an obvious way at least). 
> I’m not a fan of deferring to JSON parameters for this, although I’m 
> aware that to specify the node name of the commit target you would 
> have to use JSON again, unless qemu-img can do some translation on 
> behalf of the user: Looking at `commit -b`, that takes a filename; it 
> could make sense to have the internal QAPI parameter use a node name, 
> and qemu-img translating the filename parameter into a node name.  
> Actually, `measure` could just accept the same `-b` option as 
> `commit`, which it would then translate into the QAPI 
> `for-commit=<node-name>` option.)
And you will need to use JSON parameters if you want to specify 
something special, like for example to note that the commit target has 
discard-no-unref enabled.
I think not many people will call the command with the for_commit 
option, but it will be (always) called from code where the JSON 
parameters give much more flexibility to add some additional options 
(like discard-no-unref).

We could of course support both (JSON + for-commit=<node-name>).
But then we will also need to validate somehow that the for-commit 
node-name is equal to the target specified in the JSON somehow.
Or have something like for-commit=true/false + json or 
for-commit-node=<node-name> if you don't use a JSON?

Do you think it's worth the effort ? :)
>
> Hanna
>
>
Thanks for the review!
Jean-Louis
>
>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>> ---
>>   block/qcow2.c                    | 16 +++++++++++++
>>   include/block/block_int-common.h |  4 ++++
>>   qapi/block-core.json             | 28 ++++++++++++++++++++++
>>   qemu-img.c                       | 40 ++++++++++++++++++++++++++++----
>>   4 files changed, 83 insertions(+), 5 deletions(-)
>