Re: [PATCH v2 03/39] hmp: fix snapshot_blkdev argument type

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 :|