On Fri, Jun 26, 2026 at 01:19:04AM +0400, Marc-André Lureau wrote:
> The `snapshot-file` argument for `snapshot_blkdev` is mandatory. Correct
> the `args_type` definition to reflect this.
>
> Fixes: 6106e2492fe ("qapi: Convert blockdev_snapshot_sync")
This seems to have been an intentional design choice, rather than a
bug. In hmp_snapshot_blkdev there is code with a comment:
if (!filename) {
/* In the future, if 'snapshot-file' is not specified, the snapshot
will be taken internally. Today it's actually required. */
error_set(&errp, QERR_MISSING_PARAMETER, "snapshot-file");
hmp_handle_error(mon, &errp);
return;
}
15 years on though, the future enhancement still hasn't arrived.
IMHO the original design choice was a mistake. We should have
been it mandatory at the start, since we had freedom to relax
that later.
So I'm in favour of your change, but it should update
hmp_snapshot_blkdev to remove what will now be dead
code checking for "!filename".
> Signed-off-by: Marc-André Lureau <[email protected]>
> ---
> hmp-commands.hx | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 2b658bbbf28..1e286b7c89d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1168,7 +1168,7 @@ ERST
>
> {
> .name = "snapshot_blkdev",
> - .args_type = "reuse:-n,device:B,snapshot-file:s?,format:s?",
> + .args_type = "reuse:-n,device:B,snapshot-file:s,format:s?",
> .params = "[-n] device [new-image-file] [format]",
> .help = "initiates a live snapshot\n\t\t\t"
> "of device. If a new image file is specified,
> the\n\t\t\t"
>
> --
> 2.54.0
>
With regards,
Daniel
--
|: https://berrange.com ~~ https://hachyderm.io/@berrange :|
|: https://libvirt.org ~~ https://entangle-photo.org :|
|: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
