diff mbox

[2/3] qmp: Expose read-only option for 'change'

Message ID 1416487488-8423-3-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Nov. 20, 2014, 12:44 p.m. UTC 
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(-)

Comments

Eric Blake Nov. 26, 2014, 4:33 p.m. UTC | #1 
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>
diff mbox

Patch

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);
     if (err &&
         error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
         error_free(err);
diff --git a/qapi-schema.json b/qapi-schema.json
index 441e001..80aaa63 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1581,6 +1581,10 @@ 
 #          password to set.  If this argument is an empty string, then no future
 #          logins will be allowed.
 #
+# @read-only: #optional, only valid for block devices. This option allows
+#             changing the read-only mode of the block device; defaults to
+#             'retain'. (Since 2.3)
+#
 # Returns: Nothing on success.
 #          If @device is not a valid block device, DeviceNotFound
 #          If the new block device is encrypted, DeviceEncrypted.  Note that
@@ -1595,7 +1599,8 @@ 
 # Since: 0.14.0
 ##
 { 'command': 'change',
-  'data': {'device': 'str', 'target': 'str', '*arg': 'str'} }
+  'data': {'device': 'str', 'target': 'str', '*arg': 'str',
+           '*read-only': 'BlockdevChangeReadOnlyMode'} }
 
 ##
 # @ObjectTypeInfo:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6ef1b28..edc1783 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -109,7 +109,7 @@  EQMP
 
     {
         .name       = "change",
-        .args_type  = "device:B,target:F,arg:s?",
+        .args_type  = "device:B,target:F,arg:s?,read-only:s?",
         .mhandler.cmd_new = qmp_marshal_input_change,
     },
 
@@ -124,6 +124,8 @@  Arguments:
 - "device": device name (json-string)
 - "target": filename or item (json-string)
 - "arg": additional argument (json-string, optional)
+- "read-only": new read-only mode (json-string, optional)
+          - Possible values: "retain" (default), "ro", "rw", "auto"
 
 Examples:
 
@@ -141,6 +143,26 @@  Examples:
                             "arg": "foobar1" } }
 <- { "return": {} }
 
+3. Load a read-only medium into a writable drive
+
+-> { "execute": "change",
+             "arguments": { "device": "isa-fd0",
+                            "target": "/srv/images/ro.img",
+                            "arg": "raw",
+                            "read-only": "retain" } }
+
+<- { "error":
+     { "class": "GenericError",
+       "desc": "Could not open '/srv/images/ro.img': Permission denied" } }
+
+-> { "execute": "change",
+             "arguments": { "device": "isa-fd0",
+                            "target": "/srv/images/ro.img",
+                            "arg": "raw",
+                            "read-only": "auto" } }
+
+<- { "return": {} }
+
 EQMP
 
     {
diff --git a/qmp.c b/qmp.c
index bd63cf4..d12035b 100644
--- a/qmp.c
+++ b/qmp.c
@@ -397,13 +397,22 @@  static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
 #endif /* !CONFIG_VNC */
 
 void qmp_change(const char *device, const char *target,
-                bool has_arg, const char *arg, Error **errp)
+                bool has_arg, const char *arg,
+                bool has_read_only, BlockdevChangeReadOnlyMode read_only,
+                Error **errp)
 {
+    if (!has_read_only) {
+        read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
+    }
+
     if (strcmp(device, "vnc") == 0) {
+        if (has_read_only) {
+            error_setg(errp, "Parameter 'read-only' is invalid for VNC");
+            return;
+        }
         qmp_change_vnc(target, has_arg, arg, errp);
     } else {
-        qmp_change_blockdev(device, target, arg,
-                            BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, errp);
+        qmp_change_blockdev(device, target, arg, read_only, errp);
     }
 }