Re: [Qemu-devel] [PATCH 2/3] qmp: Expose read-only option for 'change'

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]

On 11/20/2014 05:44 AM, Max Reitz wrote:
> Expose the new read-only option of qmp_change_blockdev() for the
> 'change' QMP command. Leave it unset for HMP for now.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  hmp.c            |  2 +-
>  qapi-schema.json |  7 ++++++-
>  qmp-commands.hx  | 24 +++++++++++++++++++++++-
>  qmp.c            | 15 ++++++++++++---
>  4 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 94b27df..0719fa3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1133,7 +1133,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>          }
>      }
>  
> -    qmp_change(device, target, !!arg, arg, &err);
> +    qmp_change(device, target, !!arg, arg, false, 0, &err);

Passing raw '0' looks a bit odd for an enum value, but since the
has_read_only parameter is false, I guess it doesn't matter.

>      if (strcmp(device, "vnc") == 0) {
> +        if (has_read_only) {
> +            error_setg(errp, "Parameter 'read-only' is invalid for VNC");
> +            return;
> +        }

The 'change' command is quite ugly.  It would be nice if we could
convert it to an automatic union, where the 'device' parameter is the
discriminated union that determines whether we support or reject the
optional 'read-only' argument - except that it is not a true
discriminator (it is either 'vnc' or a device name, with the further
implication that users CANNOT name their block device 'vnc' and still be
able to use this QMP command).  I wonder if it would be better to leave
the ugly command alone, and instead introduce a new command that isn't
multiplexed.  If someone wants to set a disk's read-only status on
change, then they MUST use the nice new command, and leave the old ugly
command for backward compatibility only with no modifications to make it
even uglier.  Furthermore, adding a new command would let someone name
their block device 'vnc' (not that libvirt has any plans to do so).

At any rate, if we decide to live with extending the existing ugly
'change' command, your patch is correct, so here's a reluctant:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]