* [Qemu-devel] [PATCH] block: Catch simultaneous usage of options and their aliases
@ 2014-09-18 9:57 Kevin Wolf
2014-09-18 12:30 ` Eric Blake
0 siblings, 1 reply; 4+ messages in thread
From: Kevin Wolf @ 2014-09-18 9:57 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, armbru, stefanha
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 | 46 +++++++++++++++++++++++++++++++---------------
tests/qemu-iotests/051 | 23 +++++++++++++++++++++++
tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 99 insertions(+), 15 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index a194d04..dddd7a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -532,12 +532,22 @@ 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;
+ if (*errp) {
+ return;
+ }
+
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);
}
@@ -643,26 +653,32 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
Error *local_err = NULL;
/* 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");
+ qemu_opt_rename(all_opts, "iops", "throttling.iops-total", &local_err);
+ qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read", &local_err);
+ qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write", &local_err);
- 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");
+ qemu_opt_rename(all_opts, "bps", "throttling.bps-total", &local_err);
+ qemu_opt_rename(all_opts, "bps_rd", "throttling.bps-read", &local_err);
+ qemu_opt_rename(all_opts, "bps_wr", "throttling.bps-write", &local_err);
- 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");
+ qemu_opt_rename(all_opts, "iops_max", "throttling.iops-total-max", &local_err);
+ qemu_opt_rename(all_opts, "iops_rd_max", "throttling.iops-read-max", &local_err);
+ qemu_opt_rename(all_opts, "iops_wr_max", "throttling.iops-write-max", &local_err);
- 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");
+ qemu_opt_rename(all_opts, "bps_max", "throttling.bps-total-max", &local_err);
+ qemu_opt_rename(all_opts, "bps_rd_max", "throttling.bps-read-max", &local_err);
+ qemu_opt_rename(all_opts, "bps_wr_max", "throttling.bps-write-max", &local_err);
qemu_opt_rename(all_opts,
- "iops_size", "throttling.iops-size");
+ "iops_size", "throttling.iops-size", &local_err);
+
+ qemu_opt_rename(all_opts, "readonly", "read-only", &local_err);
- qemu_opt_rename(all_opts, "readonly", "read-only");
+ if (local_err) {
+ error_report("%s", error_get_pretty(local_err));
+ error_free(local_err);
+ return NULL;
+ }
value = qemu_opt_get(all_opts, "cache");
if (value) {
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Catch simultaneous usage of options and their aliases
2014-09-18 9:57 [Qemu-devel] [PATCH] block: Catch simultaneous usage of options and their aliases Kevin Wolf
@ 2014-09-18 12:30 ` Eric Blake
2014-09-18 14:06 ` Markus Armbruster
0 siblings, 1 reply; 4+ messages in thread
From: Eric Blake @ 2014-09-18 12:30 UTC (permalink / raw)
To: Kevin Wolf, qemu-devel; +Cc: armbru, stefanha
[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]
On 09/18/2014 03:57 AM, 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 | 46 +++++++++++++++++++++++++++++++---------------
> tests/qemu-iotests/051 | 23 +++++++++++++++++++++++
> tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 99 insertions(+), 15 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> -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;
>
> + if (*errp) {
> + return;
> + }
Not the most typical usage, so it might be worth a comment that this
function can be called with errp already set. But since it's static,
it's not too hard to figure out as-is.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Catch simultaneous usage of options and their aliases
2014-09-18 12:30 ` Eric Blake
@ 2014-09-18 14:06 ` Markus Armbruster
2014-09-18 14:34 ` Kevin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Markus Armbruster @ 2014-09-18 14:06 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-devel, stefanha
Eric Blake <eblake@redhat.com> writes:
> On 09/18/2014 03:57 AM, 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 | 46 +++++++++++++++++++++++++++++++---------------
>> tests/qemu-iotests/051 | 23 +++++++++++++++++++++++
>> tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 99 insertions(+), 15 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>>
>> -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;
>>
>> + if (*errp) {
>> + return;
>> + }
>
> Not the most typical usage, so it might be worth a comment that this
> function can be called with errp already set. But since it's static,
> it's not too hard to figure out as-is.
The problem with this usage is it doesn't combine well with the common
usage. That's why I purged it from the code back in May:
commit 297a3646c2947ee64a6d42ca264039732c6218e0
Author: Markus Armbruster <armbru@redhat.com>
Date: Wed May 7 09:53:54 2014 +0200
qapi: Replace uncommon use of the error API by the common one
We commonly use the error API like this:
err = NULL;
foo(..., &err);
if (err) {
goto out;
}
bar(..., &err);
Every error source is checked separately. The second function is only
called when the first one succeeds. Both functions are free to pass
their argument to error_set(). Because error_set() asserts no error
has been set, this effectively means they must not be called with an
error set.
The qapi-generated code uses the error API differently:
// *errp was initialized to NULL somewhere up the call chain
frob(..., errp);
gnat(..., errp);
Errors accumulate in *errp: first error wins, subsequent errors get
dropped. To make this work, the second function does nothing when
called with an error set. Requires non-null errp, or else the second
function can't see the first one fail.
This usage has also bled into visitor tests, and two device model
object property getters rtc_get_date() and balloon_stats_get_all().
With the "accumulate" technique, you need fewer error checks in
callers, and buy that with an error check in every callee. Can be
nice.
However, mixing the two techniques is confusing. You can't use the
"accumulate" technique with functions designed for the "check
separately" technique. You can use the "check separately" technique
with functions designed for the "accumulate" technique, but then
error_set() can't catch you setting an error more than once.
Standardize on the "check separately" technique for now, because it's
overwhelmingly prevalent.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
See also the thread leading up to this commit:
https://lists.nongnu.org/archive/html/qemu-devel/2014-04/msg01424.html
in particular the part starting at
https://lists.nongnu.org/archive/html/qemu-devel/2014-04/msg01830.html
I guess you're about as thrilled by what the common usage does to your
code as I am:
qemu_opt_rename(all_opts, "iops", "throttling.iops-total", &local_err);
if (local_err) {
goto fail_opt_rename;
}
qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read", &local_err);
if (local_err) {
goto fail_opt_rename;
}
qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write", &local_err);
if (local_err) {
goto fail_opt_rename;
}
[11 instances of the same pattern mercifully omitted...]
fail_opt_rename:
error_report("%s", error_get_pretty(local_err));
error_free(local_err);
return NULL;
But this code is annoyingly repetitive even before the patch! Here's
what I'd do:
static struct {
const char *from, *to;
} rename[] = {
{ "iops", "throttling.iops-total" },
{ "iops_rd", "throttling.iops-read" },
{ "iops_wr", "throttling.iops-write" },
{ "bps", "throttling.bps-total" },
{ "bps_rd", "throttling.bps-read" },
{ "bps_wr", "throttling.bps-write" },
{ "iops_max", "throttling.iops-total-max" },
{ "iops_rd_max", "throttling.iops-read-max" },
{ "iops_wr_max", "throttling.iops-write-max" },
{ "bps_max", "throttling.bps-total-max" },
{ "bps_rd_max", "throttling.bps-read-max" },
{ "bps_wr_max", "throttling.bps-write-max" },
{ "iops_size", "throttling.iops-size" },
{ "readonly", "read-only" },
};
Error *local_err = NULL;
int i;
for (i = 0; i < ARRAY_SIZE(rename); i++) {
qemu_opt_rename(all_opts, rename[i].from, rename[i].to,
&local_err);
if (local_err) {
error_report("%s", error_get_pretty(local_err));
error_free(local_err);
return NULL;
}
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Catch simultaneous usage of options and their aliases
2014-09-18 14:06 ` Markus Armbruster
@ 2014-09-18 14:34 ` Kevin Wolf
0 siblings, 0 replies; 4+ messages in thread
From: Kevin Wolf @ 2014-09-18 14:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, stefanha
Am 18.09.2014 um 16:06 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
>
> > On 09/18/2014 03:57 AM, 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 | 46 +++++++++++++++++++++++++++++++---------------
> >> tests/qemu-iotests/051 | 23 +++++++++++++++++++++++
> >> tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 99 insertions(+), 15 deletions(-)
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> >>
> >> -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;
> >>
> >> + if (*errp) {
> >> + return;
> >> + }
> >
> > Not the most typical usage, so it might be worth a comment that this
> > function can be called with errp already set. But since it's static,
> > it's not too hard to figure out as-is.
>
> The problem with this usage is it doesn't combine well with the common
> usage. That's why I purged it from the code back in May:
Oh. I didn't know that it was gone there.
> But this code is annoyingly repetitive even before the patch! Here's
> what I'd do:
>
> static struct {
> const char *from, *to;
> } rename[] = {
> { "iops", "throttling.iops-total" },
> { "iops_rd", "throttling.iops-read" },
> { "iops_wr", "throttling.iops-write" },
> { "bps", "throttling.bps-total" },
> { "bps_rd", "throttling.bps-read" },
> { "bps_wr", "throttling.bps-write" },
> { "iops_max", "throttling.iops-total-max" },
> { "iops_rd_max", "throttling.iops-read-max" },
> { "iops_wr_max", "throttling.iops-write-max" },
> { "bps_max", "throttling.bps-total-max" },
> { "bps_rd_max", "throttling.bps-read-max" },
> { "bps_wr_max", "throttling.bps-write-max" },
> { "iops_size", "throttling.iops-size" },
> { "readonly", "read-only" },
> };
> Error *local_err = NULL;
> int i;
>
> for (i = 0; i < ARRAY_SIZE(rename); i++) {
> qemu_opt_rename(all_opts, rename[i].from, rename[i].to,
> &local_err);
> if (local_err) {
> error_report("%s", error_get_pretty(local_err));
> error_free(local_err);
> return NULL;
> }
> }
Okay, fair enough.
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-18 15:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-18 9:57 [Qemu-devel] [PATCH] block: Catch simultaneous usage of options and their aliases Kevin Wolf
2014-09-18 12:30 ` Eric Blake
2014-09-18 14:06 ` Markus Armbruster
2014-09-18 14:34 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).