Re: [Qemu-devel] [PATCH RESEND 48/50] hmp: Add read-only option to change command

[Eric Blake]

On 01/27/2015 12:46 PM, Max Reitz wrote:
> Expose the new read-only option of 'blockdev-change-medium' for the
> 'change' HMP command.
> 
> Signed-off-by: Max Reitz <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  hmp-commands.hx | 20 +++++++++++++++++---
>  hmp.c           | 21 ++++++++++++++++++++-
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 

Already reviewed...

>  
> address@hidden may be used to change the read-only status of the device. It
> +accepts the following values:
> +
> address@hidden @var
> address@hidden retain
> +Retains the current status; this is the default.
> +
> address@hidden ro
> +Makes the device read-only.
> +
> address@hidden rw
> +Makes the device writable.

Hmm.  I suggested in 47/50 to rename the QMP enum values to something
longer, which would affect this on a rebase.  On the other hand, it
would be nice to make the HMP counterpart support BOTH spellings for
convenience ('read-write' for legibility, 'rw' for brevity in typing);
and I have no clue if tab completion starts to play a role either.  Up
to you if you want to add complexity or leave things simple and stupid
(the choices we make in HMP for user-friendliness have no bearing on
what libvirt will want to do, so I really have no strong preference).

...so my review still stands if nothing changes, and is probably worth
dropping if you respin to add user-friendly complexity.


>      const char *arg = qdict_get_try_str(qdict, "arg");
> +    const char *read_only = qdict_get_try_str(qdict, "read-only");
> +    BlockdevChangeReadOnlyMode read_only_mode = 0;
>      Error *err = NULL;
>  
>      if (strcmp(device, "vnc") == 0) {
> +        if (read_only) {
> +            monitor_printf(mon, "Parameter 'read-only' is invalid for VNC");

Hmm. In HMP, you don't type the parameter name (it is implicit based on
the rules for converting positional HMP arguments into dictionary
entries for use in the rest of the code).  In particular, that means
that if I type 'change vnc password rw', that means that 'rw' will be in
the "read-only" parameter position, even though it has nothing to do
with read-only.  [hmm - how did 'change vnc password XXX' work anyways,
since the .params for 'change' didn't allow a third argument before your
patch?]

At any rate, the idea I mentioned in the QMP patch about naming the
parameter 'read-mode' instead of 'read-only' might make for nicer error
messages.

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

signature.asc
Description: OpenPGP digital signature