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

[Max Reitz]

On 2015-01-28 at 16:22, Eric Blake wrote:
> 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.

I probably won't care for brevity too much. Again, if we want to allow 
shortcuts later on, it won't break compatibility so we don't really need 
to worry about it now (although I guess if we don't add it now, we won't 
add it ever).

> >       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?]

Well, "help change" will tell you that that parameter is named 
"read-only", so I think mentioning the parameter name (instead of 
"garbage at the end of line" or something like that) is fine.

Max

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