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-lo...@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(-)
