[Qemu-devel] [PATCH v2 0/2] block: Catch simultaneous usage of options and their aliases

[Qemu-devel] [PATCH v2 0/2] block: Catch simultaneous usage of options and their aliases

[Kevin Wolf]

v2:
- Remove QAPI-style use of errp, convert to table to keep things
  readable anyway [Markus]

Kevin Wolf (2):
  block: Specify -drive legacy option aliases in array
  block: Catch simultaneous usage of options and their aliases

 blockdev.c                 | 53 ++++++++++++++++++++++++++++++++--------------
 tests/qemu-iotests/051     | 23 ++++++++++++++++++++
 tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] block: Specify -drive legacy option aliases in array

[Kevin Wolf]

Instead of a series of qemu_opt_rename() calls, use an array that
contains all of the renames and call qemu_opt_rename() in a loop. This
will keep the code readable even when we add an error return to
qemu_opt_rename().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a194d04..6a33fd2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -641,28 +641,37 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     const char *serial;
     const char *filename;
     Error *local_err = NULL;
+    int i;
 
     /* Change legacy command line options into QMP ones */
-    qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
-    qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
-    qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write");
+    static const struct {
+        const char *from;
+        const char *to;
+    } opt_renames[] = {
+        { "iops",           "throttling.iops-total" },
+        { "iops_rd",        "throttling.iops-read" },
+        { "iops_wr",        "throttling.iops-write" },
 
-    qemu_opt_rename(all_opts, "bps", "throttling.bps-total");
-    qemu_opt_rename(all_opts, "bps_rd", "throttling.bps-read");
-    qemu_opt_rename(all_opts, "bps_wr", "throttling.bps-write");
+        { "bps",            "throttling.bps-total" },
+        { "bps_rd",         "throttling.bps-read" },
+        { "bps_wr",         "throttling.bps-write" },
 
-    qemu_opt_rename(all_opts, "iops_max", "throttling.iops-total-max");
-    qemu_opt_rename(all_opts, "iops_rd_max", "throttling.iops-read-max");
-    qemu_opt_rename(all_opts, "iops_wr_max", "throttling.iops-write-max");
+        { "iops_max",       "throttling.iops-total-max" },
+        { "iops_rd_max",    "throttling.iops-read-max" },
+        { "iops_wr_max",    "throttling.iops-write-max" },
 
-    qemu_opt_rename(all_opts, "bps_max", "throttling.bps-total-max");
-    qemu_opt_rename(all_opts, "bps_rd_max", "throttling.bps-read-max");
-    qemu_opt_rename(all_opts, "bps_wr_max", "throttling.bps-write-max");
+        { "bps_max",        "throttling.bps-total-max" },
+        { "bps_rd_max",     "throttling.bps-read-max" },
+        { "bps_wr_max",     "throttling.bps-write-max" },
 
-    qemu_opt_rename(all_opts,
-                    "iops_size", "throttling.iops-size");
+        { "iops_size",      "throttling.iops-size" },
 
-    qemu_opt_rename(all_opts, "readonly", "read-only");
+        { "readonly",       "read-only" },
+    };
+
+    for (i = 0; i < ARRAY_SIZE(opt_renames); i++) {
+        qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to);
+    }
 
     value = qemu_opt_get(all_opts, "cache");
     if (value) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] block: Catch simultaneous usage of options and their aliases

[Kevin Wolf]

While thinking about precedence of conflicting block device options from
different sources, I noticed that you can specify both an option and its
legacy alias at the same time (e.g. readonly=on,read-only=off). Rather
than specifying the order of precedence, we should simply forbid such
combinations.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c                 | 16 ++++++++++++++--
 tests/qemu-iotests/051     | 23 +++++++++++++++++++++++
 tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6a33fd2..7adecae 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -532,12 +532,18 @@ err_no_opts:
     return NULL;
 }
 
-static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
+static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
+                            Error **errp)
 {
     const char *value;
 
     value = qemu_opt_get(opts, from);
     if (value) {
+        if (qemu_opt_find(opts, to)) {
+            error_setg(errp, "'%s' and its alias '%s' can't be used at the "
+                       "same time", to, from);
+            return;
+        }
         qemu_opt_set(opts, to, value);
         qemu_opt_unset(opts, from);
     }
@@ -670,7 +676,13 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     };
 
     for (i = 0; i < ARRAY_SIZE(opt_renames); i++) {
-        qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to);
+        qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to,
+                        &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
+            return NULL;
+        }
     }
 
     value = qemu_opt_get(all_opts, "cache");
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index a41334e..11c858f 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -199,6 +199,29 @@ run_qemu -drive file.driver=raw
 run_qemu -drive foo=bar
 
 echo
+echo === Specifying both an option and its legacy alias ===
+echo
+
+run_qemu -drive file="$TEST_IMG",iops=1234,throttling.iops-total=5678
+run_qemu -drive file="$TEST_IMG",iops_rd=1234,throttling.iops-read=5678
+run_qemu -drive file="$TEST_IMG",iops_wr=1234,throttling.iops-write=5678
+
+run_qemu -drive file="$TEST_IMG",bps=1234,throttling.bps-total=5678
+run_qemu -drive file="$TEST_IMG",bps_rd=1234,throttling.bps-read=5678
+run_qemu -drive file="$TEST_IMG",bps_wr=1234,throttling.bps-write=5678
+
+run_qemu -drive file="$TEST_IMG",iops_max=1234,throttling.iops-total-max=5678
+run_qemu -drive file="$TEST_IMG",iops_rd_max=1234,throttling.iops-read-max=5678
+run_qemu -drive file="$TEST_IMG",iops_wr_max=1234,throttling.iops-write-max=5678
+
+run_qemu -drive file="$TEST_IMG",bps_max=1234,throttling.bps-total-max=5678
+run_qemu -drive file="$TEST_IMG",bps_rd_max=1234,throttling.bps-read-max=5678
+run_qemu -drive file="$TEST_IMG",bps_wr_max=1234,throttling.bps-write-max=5678
+
+run_qemu -drive file="$TEST_IMG",iops_size=1234,throttling.iops-size=5678
+run_qemu -drive file="$TEST_IMG",readonly=on,read-only=off
+
+echo
 echo === Parsing protocol from file name ===
 echo
 
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index fee597e..7f16134 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -275,6 +275,51 @@ Testing: -drive foo=bar
 QEMU_PROG: -drive foo=bar: could not open disk image ide0-hd0: Must specify either driver or file
 
 
+=== Specifying both an option and its legacy alias ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops=1234,throttling.iops-total=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops=1234,throttling.iops-total=5678: 'throttling.iops-total' and its alias 'iops' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops_rd=1234,throttling.iops-read=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops_rd=1234,throttling.iops-read=5678: 'throttling.iops-read' and its alias 'iops_rd' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops_wr=1234,throttling.iops-write=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops_wr=1234,throttling.iops-write=5678: 'throttling.iops-write' and its alias 'iops_wr' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=1234,throttling.bps-total=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=1234,throttling.bps-total=5678: 'throttling.bps-total' and its alias 'bps' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd=1234,throttling.bps-read=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd=1234,throttling.bps-read=5678: 'throttling.bps-read' and its alias 'bps_rd' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_wr=1234,throttling.bps-write=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_wr=1234,throttling.bps-write=5678: 'throttling.bps-write' and its alias 'bps_wr' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops_max=1234,throttling.iops-total-max=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops_max=1234,throttling.iops-total-max=5678: 'throttling.iops-total-max' and its alias 'iops_max' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops_rd_max=1234,throttling.iops-read-max=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops_rd_max=1234,throttling.iops-read-max=5678: 'throttling.iops-read-max' and its alias 'iops_rd_max' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops_wr_max=1234,throttling.iops-write-max=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops_wr_max=1234,throttling.iops-write-max=5678: 'throttling.iops-write-max' and its alias 'iops_wr_max' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_max=1234,throttling.bps-total-max=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_max=1234,throttling.bps-total-max=5678: 'throttling.bps-total-max' and its alias 'bps_max' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd_max=1234,throttling.bps-read-max=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd_max=1234,throttling.bps-read-max=5678: 'throttling.bps-read-max' and its alias 'bps_rd_max' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_wr_max=1234,throttling.bps-write-max=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_wr_max=1234,throttling.bps-write-max=5678: 'throttling.bps-write-max' and its alias 'bps_wr_max' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops_size=1234,throttling.iops-size=5678
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops_size=1234,throttling.iops-size=5678: 'throttling.iops-size' and its alias 'iops_size' can't be used at the same time
+
+Testing: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off: 'read-only' and its alias 'readonly' can't be used at the same time
+
+
 === Parsing protocol from file name ===
 
 Testing: -hda foo:bar
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/2] block: Specify -drive legacy option aliases in array

[Benoît Canet]

The Wednesday 24 Sep 2014 à 16:46:28 (+0200), Kevin Wolf wrote :
> Instead of a series of qemu_opt_rename() calls, use an array that
> contains all of the renames and call qemu_opt_rename() in a loop. This
> will keep the code readable even when we add an error return to
> qemu_opt_rename().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a194d04..6a33fd2 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -641,28 +641,37 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      const char *serial;
>      const char *filename;
>      Error *local_err = NULL;
> +    int i;
>  
>      /* Change legacy command line options into QMP ones */
> -    qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
> -    qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
> -    qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write");
> +    static const struct {
> +        const char *from;
> +        const char *to;
> +    } opt_renames[] = {
> +        { "iops",           "throttling.iops-total" },
> +        { "iops_rd",        "throttling.iops-read" },
> +        { "iops_wr",        "throttling.iops-write" },
>  
> -    qemu_opt_rename(all_opts, "bps", "throttling.bps-total");
> -    qemu_opt_rename(all_opts, "bps_rd", "throttling.bps-read");
> -    qemu_opt_rename(all_opts, "bps_wr", "throttling.bps-write");
> +        { "bps",            "throttling.bps-total" },
> +        { "bps_rd",         "throttling.bps-read" },
> +        { "bps_wr",         "throttling.bps-write" },
>  
> -    qemu_opt_rename(all_opts, "iops_max", "throttling.iops-total-max");
> -    qemu_opt_rename(all_opts, "iops_rd_max", "throttling.iops-read-max");
> -    qemu_opt_rename(all_opts, "iops_wr_max", "throttling.iops-write-max");
> +        { "iops_max",       "throttling.iops-total-max" },
> +        { "iops_rd_max",    "throttling.iops-read-max" },
> +        { "iops_wr_max",    "throttling.iops-write-max" },
>  
> -    qemu_opt_rename(all_opts, "bps_max", "throttling.bps-total-max");
> -    qemu_opt_rename(all_opts, "bps_rd_max", "throttling.bps-read-max");
> -    qemu_opt_rename(all_opts, "bps_wr_max", "throttling.bps-write-max");
> +        { "bps_max",        "throttling.bps-total-max" },
> +        { "bps_rd_max",     "throttling.bps-read-max" },
> +        { "bps_wr_max",     "throttling.bps-write-max" },
>  
> -    qemu_opt_rename(all_opts,
> -                    "iops_size", "throttling.iops-size");
> +        { "iops_size",      "throttling.iops-size" },
>  
> -    qemu_opt_rename(all_opts, "readonly", "read-only");
> +        { "readonly",       "read-only" },
> +    };
> +
> +    for (i = 0; i < ARRAY_SIZE(opt_renames); i++) {
> +        qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to);
> +    }
>  
>      value = qemu_opt_get(all_opts, "cache");
>      if (value) {
> -- 
> 1.8.3.1
> 
> 

Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Re: [Qemu-devel] [PATCH v2 2/2] block: Catch simultaneous usage of options and their aliases

[Benoît Canet]

The Wednesday 24 Sep 2014 à 16:46:29 (+0200), Kevin Wolf wrote :
> While thinking about precedence of conflicting block device options from
> different sources, I noticed that you can specify both an option and its
> legacy alias at the same time (e.g. readonly=on,read-only=off). Rather
> than specifying the order of precedence, we should simply forbid such
> combinations.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c                 | 16 ++++++++++++++--
>  tests/qemu-iotests/051     | 23 +++++++++++++++++++++++
>  tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 6a33fd2..7adecae 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -532,12 +532,18 @@ err_no_opts:
>      return NULL;
>  }
>  
> -static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
> +static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
> +                            Error **errp)
>  {
>      const char *value;
>  
>      value = qemu_opt_get(opts, from);
>      if (value) {
> +        if (qemu_opt_find(opts, to)) {
> +            error_setg(errp, "'%s' and its alias '%s' can't be used at the "

Maybe specify we are talking about options in the error message so the user is
provided with a bit of context about what's happening.

> +            error_setg(errp, "the option '%s' and its alias '%s' can't be used at the "
> +                       "same time", to, from);
> +            return;
> +        }

Anyway:

Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Re: [Qemu-devel] [PATCH v2 0/2] block: Catch simultaneous usage of options and their aliases

[Markus Armbruster]

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/2] block: Catch simultaneous usage of options and their aliases

[Kevin Wolf]

Am 24.09.2014 um 16:46 hat Kevin Wolf geschrieben:
> v2:
> - Remove QAPI-style use of errp, convert to table to keep things
>   readable anyway [Markus]
> 
> Kevin Wolf (2):
>   block: Specify -drive legacy option aliases in array
>   block: Catch simultaneous usage of options and their aliases

Applied to the block branch.

Kevin