[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RESEND 48/50] hmp: Add read-only option to chang
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH RESEND 48/50] hmp: Add read-only option to change command |
Date: |
Wed, 28 Jan 2015 14:22:45 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
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
- [Qemu-devel] [PATCH RESEND 44/50] block: Inquire tray state before tray-moved events, (continued)
- [Qemu-devel] [PATCH RESEND 50/50] iotests: Add test for change-related QMP commands, Max Reitz, 2015/01/27
- [Qemu-devel] [PATCH RESEND 48/50] hmp: Add read-only option to change command, Max Reitz, 2015/01/27
- Re: [Qemu-devel] [PATCH RESEND 48/50] hmp: Add read-only option to change command,
Eric Blake <=
- [Qemu-devel] [PATCH RESEND 03/50] hw/block/fdc: Implement tray status, Max Reitz, 2015/01/27
- [Qemu-devel] [PATCH RESEND 17/50] block: Respect empty BB in bdrv_lookup_bs(), Max Reitz, 2015/01/27
- [Qemu-devel] [PATCH RESEND 37/50] block: Add blk_remove_bs(), Max Reitz, 2015/01/27
- [Qemu-devel] [PATCH RESEND 20/50] blockdev: Check blk_is_available() in sn-del-int-sync, Max Reitz, 2015/01/27