* [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
@ 2014-02-19 15:12 ` Kevin Wolf
2014-02-19 16:05 ` Fam Zheng
2014-02-19 16:38 ` Eric Blake
2014-02-19 15:12 ` [Qemu-devel] [PATCH 2/7] qemu-img convert: " Kevin Wolf
` (6 subsequent siblings)
7 siblings, 2 replies; 15+ messages in thread
From: Kevin Wolf @ 2014-02-19 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
If you specified multiple -o options for qemu-img create, it would
silently ignore all but the last one. Similarly, for other options the
last occurence wins (which is at least a bit less surprising). Error out
instead.
The only exception is a -o help option, which may be added to any valid
qemu-img create command and ignores all other options.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index c989850..6a64fe1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -341,13 +341,14 @@ static int img_create(int argc, char **argv)
{
int c;
uint64_t img_size = -1;
- const char *fmt = "raw";
+ const char *fmt = NULL;
const char *base_fmt = NULL;
const char *filename;
const char *base_filename = NULL;
char *options = NULL;
Error *local_err = NULL;
bool quiet = false;
+ bool options_help = false;
for(;;) {
c = getopt(argc, argv, "F:b:f:he6o:q");
@@ -360,12 +361,24 @@ static int img_create(int argc, char **argv)
help();
break;
case 'F':
+ if (base_fmt) {
+ error_report("-F may only be specified once");
+ return 1;
+ }
base_fmt = optarg;
break;
case 'b':
+ if (base_filename) {
+ error_report("-b may only be specified once");
+ return 1;
+ }
base_filename = optarg;
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case 'e':
@@ -377,7 +390,16 @@ static int img_create(int argc, char **argv)
"compat6\' instead!");
return 1;
case 'o':
- options = optarg;
+ if (is_help_option(optarg)) {
+ options_help = true;
+ } else if (!options) {
+ options = optarg;
+ } else {
+ error_report("-o cannot be used multiple times. Please use a "
+ "single -o option with comma-separated settings "
+ "instead.");
+ return 1;
+ }
break;
case 'q':
quiet = true;
@@ -385,6 +407,10 @@ static int img_create(int argc, char **argv)
}
}
+ if (!fmt) {
+ fmt = "raw";
+ }
+
/* Get the filename */
if (optind >= argc) {
help();
@@ -413,7 +439,7 @@ static int img_create(int argc, char **argv)
help();
}
- if (options && is_help_option(options)) {
+ if (options_help) {
return print_block_option_help(filename, fmt);
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once
2014-02-19 15:12 ` [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once Kevin Wolf
@ 2014-02-19 16:05 ` Fam Zheng
2014-02-19 16:38 ` Eric Blake
1 sibling, 0 replies; 15+ messages in thread
From: Fam Zheng @ 2014-02-19 16:05 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, mreitz
On Wed, 02/19 16:12, Kevin Wolf wrote:
> If you specified multiple -o options for qemu-img create, it would
> silently ignore all but the last one. Similarly, for other options the
> last occurence wins (which is at least a bit less surprising). Error out
> instead.
>
> The only exception is a -o help option, which may be added to any valid
> qemu-img create command and ignores all other options.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qemu-img.c | 32 +++++++++++++++++++++++++++++---
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index c989850..6a64fe1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -341,13 +341,14 @@ static int img_create(int argc, char **argv)
> {
> int c;
> uint64_t img_size = -1;
> - const char *fmt = "raw";
> + const char *fmt = NULL;
> const char *base_fmt = NULL;
> const char *filename;
> const char *base_filename = NULL;
> char *options = NULL;
> Error *local_err = NULL;
> bool quiet = false;
> + bool options_help = false;
>
> for(;;) {
> c = getopt(argc, argv, "F:b:f:he6o:q");
> @@ -360,12 +361,24 @@ static int img_create(int argc, char **argv)
> help();
> break;
> case 'F':
> + if (base_fmt) {
> + error_report("-F may only be specified once");
> + return 1;
> + }
> base_fmt = optarg;
> break;
> case 'b':
> + if (base_filename) {
> + error_report("-b may only be specified once");
> + return 1;
> + }
> base_filename = optarg;
> break;
> case 'f':
> + if (fmt) {
> + error_report("-f may only be specified once");
> + return 1;
> + }
> fmt = optarg;
> break;
> case 'e':
> @@ -377,7 +390,16 @@ static int img_create(int argc, char **argv)
> "compat6\' instead!");
> return 1;
> case 'o':
> - options = optarg;
> + if (is_help_option(optarg)) {
> + options_help = true;
> + } else if (!options) {
> + options = optarg;
> + } else {
> + error_report("-o cannot be used multiple times. Please use a "
> + "single -o option with comma-separated settings "
> + "instead.");
Not consistent with ending "." but not a big problem.
Reviewed-by: Fam Zheng <famz@redhat.com>
> + return 1;
> + }
> break;
> case 'q':
> quiet = true;
> @@ -385,6 +407,10 @@ static int img_create(int argc, char **argv)
> }
> }
>
> + if (!fmt) {
> + fmt = "raw";
> + }
> +
> /* Get the filename */
> if (optind >= argc) {
> help();
> @@ -413,7 +439,7 @@ static int img_create(int argc, char **argv)
> help();
> }
>
> - if (options && is_help_option(options)) {
> + if (options_help) {
> return print_block_option_help(filename, fmt);
> }
>
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once
2014-02-19 15:12 ` [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once Kevin Wolf
2014-02-19 16:05 ` Fam Zheng
@ 2014-02-19 16:38 ` Eric Blake
1 sibling, 0 replies; 15+ messages in thread
From: Eric Blake @ 2014-02-19 16:38 UTC (permalink / raw)
To: Kevin Wolf, qemu-devel; +Cc: stefanha, mreitz
[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]
On 02/19/2014 08:12 AM, Kevin Wolf wrote:
> If you specified multiple -o options for qemu-img create, it would
> silently ignore all but the last one. Similarly, for other options the
> last occurence wins (which is at least a bit less surprising). Error out
s/occurence/occurrence/
> instead.
For other options, erroring out is okay. But for -o, I would prefer if
we could concatenate multiple -o as if they had been passed in one
larger -o, rather than erroring out. That is, I'd rather treat:
-o backing_file=/path/to/foo -o backing_fmt=qcow2
as a synonym of
-o backing_file=/path/to/foo,backing_fmt=qcow2
>
> The only exception is a -o help option, which may be added to any valid
> qemu-img create command and ignores all other options.
If you consider my above request for concatenating multiple -o options,
rather than rejecting duplicates, then that means:
-o backing_file=/path/to/foo,help
would have to do the right thing about displaying help.
I also have the complaint that:
qemu-img create -o help -f qcow2
does not tell me the qcow2 specific options; I have to do something like:
qemu-img create -o help -f qcow2 /dev/null
It would be nice if we could make -o help smarter so that it no longer
requires the presence of a file name (probably a separate patch).
In other words, I like that your series is trying to improve things, but
I don't think it is making the right improvement.
--
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: 604 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/7] qemu-img convert: Detect options specified more than once
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
2014-02-19 15:12 ` [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once Kevin Wolf
@ 2014-02-19 15:12 ` Kevin Wolf
2014-02-19 15:12 ` [Qemu-devel] [PATCH 3/7] qemu-img amend: " Kevin Wolf
` (5 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2014-02-19 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Instead of ignoring all option values but the last one, error out if an
option is set multiple times.
Again, the only exception is a -o help option, which may be added to any
valid qemu-img convert command and ignores all other options.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 6a64fe1..55c2c75 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1170,6 +1170,7 @@ static int img_convert(int argc, char **argv)
QEMUOptionParameter *param = NULL, *create_options = NULL;
QEMUOptionParameter *out_baseimg_param;
char *options = NULL;
+ bool options_help = false;
const char *snapshot_name = NULL;
int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
bool quiet = false;
@@ -1177,8 +1178,8 @@ static int img_convert(int argc, char **argv)
QemuOpts *sn_opts = NULL;
fmt = NULL;
- out_fmt = "raw";
- cache = "unsafe";
+ out_fmt = NULL;
+ cache = NULL;
out_baseimg = NULL;
compress = 0;
skip_create = 0;
@@ -1193,12 +1194,24 @@ static int img_convert(int argc, char **argv)
help();
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case 'O':
+ if (out_fmt) {
+ error_report("-O may only be specified once");
+ return 1;
+ }
out_fmt = optarg;
break;
case 'B':
+ if (out_baseimg) {
+ error_report("-B may only be specified once");
+ return 1;
+ }
out_baseimg = optarg;
break;
case 'c':
@@ -1213,12 +1226,29 @@ static int img_convert(int argc, char **argv)
"compat6\' instead!");
return 1;
case 'o':
- options = optarg;
+ if (is_help_option(optarg)) {
+ options_help = true;
+ } else if (!options) {
+ options = optarg;
+ } else {
+ error_report("-o cannot be used multiple times. Please use a "
+ "single -o option with comma-separated settings "
+ "instead.");
+ return 1;
+ }
break;
case 's':
+ if (sn_opts || snapshot_name) {
+ error_report("-l/-s may only be specified once");
+ return 1;
+ }
snapshot_name = optarg;
break;
case 'l':
+ if (sn_opts || snapshot_name) {
+ error_report("-l/-s may only be specified once");
+ return 1;
+ }
if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
if (!sn_opts) {
@@ -1247,6 +1277,10 @@ static int img_convert(int argc, char **argv)
progress = 1;
break;
case 't':
+ if (cache) {
+ error_report("-t may only be specified once");
+ return 1;
+ }
cache = optarg;
break;
case 'q':
@@ -1258,6 +1292,13 @@ static int img_convert(int argc, char **argv)
}
}
+ if (!out_fmt) {
+ out_fmt = "raw";
+ }
+ if (!cache) {
+ cache = "unsafe";
+ }
+
if (quiet) {
progress = 0;
}
@@ -1272,7 +1313,7 @@ static int img_convert(int argc, char **argv)
/* Initialize before goto out */
qemu_progress_init(progress, 1.0);
- if (options && is_help_option(options)) {
+ if (options_help) {
ret = print_block_option_help(out_filename, out_fmt);
goto out;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 3/7] qemu-img amend: Detect options specified more than once
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
2014-02-19 15:12 ` [Qemu-devel] [PATCH 1/7] qemu-img create: Detect options specified more than once Kevin Wolf
2014-02-19 15:12 ` [Qemu-devel] [PATCH 2/7] qemu-img convert: " Kevin Wolf
@ 2014-02-19 15:12 ` Kevin Wolf
2014-02-19 15:12 ` [Qemu-devel] [PATCH 4/7] qemu-img: " Kevin Wolf
` (4 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2014-02-19 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Instead of ignoring all option values but the last one, error out if an
option is set multiple times.
Again, the only exception is a -o help option, which may be added to any
valid qemu-img amend command and ignores all other options.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 55c2c75..247987d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2687,6 +2687,7 @@ static int img_amend(int argc, char **argv)
{
int c, ret = 0;
char *options = NULL;
+ bool options_help = false;
QEMUOptionParameter *create_options = NULL, *options_param = NULL;
const char *fmt = NULL, *filename;
bool quiet = false;
@@ -2704,9 +2705,22 @@ static int img_amend(int argc, char **argv)
help();
break;
case 'o':
- options = optarg;
+ if (is_help_option(optarg)) {
+ options_help = true;
+ } else if (!options) {
+ options = optarg;
+ } else {
+ error_report("-o cannot be used multiple times. Please use a "
+ "single -o option with comma-separated settings "
+ "instead.");
+ return 1;
+ }
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case 'q':
@@ -2734,7 +2748,7 @@ static int img_amend(int argc, char **argv)
fmt = bs->drv->format_name;
- if (is_help_option(options)) {
+ if (options_help) {
ret = print_block_option_help(filename, fmt);
goto out;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 4/7] qemu-img: Detect options specified more than once
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
` (2 preceding siblings ...)
2014-02-19 15:12 ` [Qemu-devel] [PATCH 3/7] qemu-img amend: " Kevin Wolf
@ 2014-02-19 15:12 ` Kevin Wolf
2014-02-19 15:12 ` [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options Kevin Wolf
` (3 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2014-02-19 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Instead of ignoring all option values but the last one, error out if an
option is set multiple times. These are the simpler subcommands without
-o, so the patch becomes a bit easier and we can fix the remaining
subcommands in a single patch.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/qemu-img.c b/qemu-img.c
index 247987d..a889c84 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -594,6 +594,10 @@ static int img_check(int argc, char **argv)
help();
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case 'r':
@@ -608,6 +612,10 @@ static int img_check(int argc, char **argv)
}
break;
case OPTION_OUTPUT:
+ if (output) {
+ error_report("--output may only be specified once");
+ return 1;
+ }
output = optarg;
break;
case 'q':
@@ -716,9 +724,17 @@ static int img_commit(int argc, char **argv)
help();
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case 't':
+ if (cache) {
+ error_report("-t may only be specified once");
+ return 1;
+ }
cache = optarg;
break;
case 'q':
@@ -949,9 +965,17 @@ static int img_compare(int argc, char **argv)
help();
break;
case 'f':
+ if (fmt1) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt1 = optarg;
break;
case 'F':
+ if (fmt2) {
+ error_report("-F may only be specified once");
+ return 1;
+ }
fmt2 = optarg;
break;
case 'p':
@@ -1906,9 +1930,17 @@ static int img_info(int argc, char **argv)
help();
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case OPTION_OUTPUT:
+ if (output) {
+ error_report("--output may only be specified once");
+ return 1;
+ }
output = optarg;
break;
case OPTION_BACKING_CHAIN:
@@ -2074,9 +2106,17 @@ static int img_map(int argc, char **argv)
help();
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case OPTION_OUTPUT:
+ if (output) {
+ error_report("--output may only be specified once");
+ return 1;
+ }
output = optarg;
break;
}
@@ -2282,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
/* Parse commandline parameters */
fmt = NULL;
- cache = BDRV_DEFAULT_CACHE;
+ cache = NULL;
out_baseimg = NULL;
out_basefmt = NULL;
for(;;) {
@@ -2296,12 +2336,24 @@ static int img_rebase(int argc, char **argv)
help();
return 0;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case 'F':
+ if (out_basefmt) {
+ error_report("-F may only be specified once");
+ return 1;
+ }
out_basefmt = optarg;
break;
case 'b':
+ if (out_baseimg) {
+ error_report("-b may only be specified once");
+ return 1;
+ }
out_baseimg = optarg;
break;
case 'u':
@@ -2311,6 +2363,10 @@ static int img_rebase(int argc, char **argv)
progress = 1;
break;
case 't':
+ if (cache) {
+ error_report("-t may only be specified once");
+ return 1;
+ }
cache = optarg;
break;
case 'q':
@@ -2319,6 +2375,10 @@ static int img_rebase(int argc, char **argv)
}
}
+ if (!cache) {
+ cache = BDRV_DEFAULT_CACHE;
+ }
+
if (quiet) {
progress = 0;
}
@@ -2603,6 +2663,10 @@ static int img_resize(int argc, char **argv)
help();
break;
case 'f':
+ if (fmt) {
+ error_report("-f may only be specified once");
+ return 1;
+ }
fmt = optarg;
break;
case 'q':
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
` (3 preceding siblings ...)
2014-02-19 15:12 ` [Qemu-devel] [PATCH 4/7] qemu-img: " Kevin Wolf
@ 2014-02-19 15:12 ` Kevin Wolf
2014-02-19 16:18 ` Fam Zheng
2014-02-19 16:46 ` Eric Blake
2014-02-19 15:12 ` [Qemu-devel] [PATCH 6/7] qemu-img convert: " Kevin Wolf
` (2 subsequent siblings)
7 siblings, 2 replies; 15+ messages in thread
From: Kevin Wolf @ 2014-02-19 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Multiple -o options has the same meaning as having a single option with
all settings in the order of their respective -o options.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index a889c84..30273ad 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -384,21 +384,18 @@ static int img_create(int argc, char **argv)
case 'e':
error_report("option -e is deprecated, please use \'-o "
"encryption\' instead!");
- return 1;
+ goto fail;
case '6':
error_report("option -6 is deprecated, please use \'-o "
"compat6\' instead!");
- return 1;
+ goto fail;
case 'o':
if (is_help_option(optarg)) {
options_help = true;
} else if (!options) {
- options = optarg;
+ options = g_strdup(optarg);
} else {
- error_report("-o cannot be used multiple times. Please use a "
- "single -o option with comma-separated settings "
- "instead.");
- return 1;
+ options = g_strdup_printf("%s,%s", options, optarg);
}
break;
case 'q':
@@ -431,7 +428,7 @@ static int img_create(int argc, char **argv)
error_report("kilobytes, megabytes, gigabytes, terabytes, "
"petabytes and exabytes.");
}
- return 1;
+ goto fail;
}
img_size = (uint64_t)sval;
}
@@ -440,6 +437,7 @@ static int img_create(int argc, char **argv)
}
if (options_help) {
+ g_free(options);
return print_block_option_help(filename, fmt);
}
@@ -448,10 +446,15 @@ static int img_create(int argc, char **argv)
if (error_is_set(&local_err)) {
error_report("%s: %s", filename, error_get_pretty(local_err));
error_free(local_err);
- return 1;
+ goto fail;
}
+ g_free(options);
return 0;
+
+fail:
+ g_free(options);
+ return 1;
}
static void dump_json_image_check(ImageCheck *check, bool quiet)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options
2014-02-19 15:12 ` [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options Kevin Wolf
@ 2014-02-19 16:18 ` Fam Zheng
2014-02-19 16:46 ` Eric Blake
1 sibling, 0 replies; 15+ messages in thread
From: Fam Zheng @ 2014-02-19 16:18 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, mreitz
On Wed, 02/19 16:12, Kevin Wolf wrote:
> Multiple -o options has the same meaning as having a single option with
> all settings in the order of their respective -o options.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qemu-img.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index a889c84..30273ad 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -384,21 +384,18 @@ static int img_create(int argc, char **argv)
> case 'e':
> error_report("option -e is deprecated, please use \'-o "
> "encryption\' instead!");
> - return 1;
> + goto fail;
> case '6':
> error_report("option -6 is deprecated, please use \'-o "
> "compat6\' instead!");
> - return 1;
> + goto fail;
> case 'o':
> if (is_help_option(optarg)) {
> options_help = true;
> } else if (!options) {
> - options = optarg;
> + options = g_strdup(optarg);
> } else {
> - error_report("-o cannot be used multiple times. Please use a "
> - "single -o option with comma-separated settings "
> - "instead.");
> - return 1;
> + options = g_strdup_printf("%s,%s", options, optarg);
OK. So what's done in patch 1 is masked here. Why not squash them and avoid the
intermediate state? And, isn't original options leaked here?
Fam
> }
> break;
> case 'q':
> @@ -431,7 +428,7 @@ static int img_create(int argc, char **argv)
> error_report("kilobytes, megabytes, gigabytes, terabytes, "
> "petabytes and exabytes.");
> }
> - return 1;
> + goto fail;
> }
> img_size = (uint64_t)sval;
> }
> @@ -440,6 +437,7 @@ static int img_create(int argc, char **argv)
> }
>
> if (options_help) {
> + g_free(options);
> return print_block_option_help(filename, fmt);
> }
>
> @@ -448,10 +446,15 @@ static int img_create(int argc, char **argv)
> if (error_is_set(&local_err)) {
> error_report("%s: %s", filename, error_get_pretty(local_err));
> error_free(local_err);
> - return 1;
> + goto fail;
> }
>
> + g_free(options);
> return 0;
> +
> +fail:
> + g_free(options);
> + return 1;
> }
>
> static void dump_json_image_check(ImageCheck *check, bool quiet)
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options
2014-02-19 15:12 ` [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options Kevin Wolf
2014-02-19 16:18 ` Fam Zheng
@ 2014-02-19 16:46 ` Eric Blake
1 sibling, 0 replies; 15+ messages in thread
From: Eric Blake @ 2014-02-19 16:46 UTC (permalink / raw)
To: Kevin Wolf, qemu-devel; +Cc: stefanha, mreitz
[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]
On 02/19/2014 08:12 AM, Kevin Wolf wrote:
> Multiple -o options has the same meaning as having a single option with
> all settings in the order of their respective -o options.
Ah, this addresses (part of) the problem I raised about patch 1. But it
feels a bit awkward to have the intermediate state.
> case 'o':
> if (is_help_option(optarg)) {
> options_help = true;
> } else if (!options) {
> - options = optarg;
> + options = g_strdup(optarg);
> } else {
> - error_report("-o cannot be used multiple times. Please use a "
> - "single -o option with comma-separated settings "
> - "instead.");
> - return 1;
> + options = g_strdup_printf("%s,%s", options, optarg);
In addition to the memleak that Fam pointed out, I still think you have
the problem that you aren't detecting:
-o backing_file=/path/to/foo,help
as specifying options_help. I think you instead need to collect ALL -o
options without special-casing options_help, then at the end, call
contains_help_option which looks for "help" or "?" among all the
comma-separated options.
>
> + g_free(options);
> return 0;
> +
> +fail:
> + g_free(options);
> + return 1;
> }
I'd also like if we started using EXIT_FAILURE instead of the magic
number 1 (but that's probably a separate cleanup).
--
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: 604 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 6/7] qemu-img convert: Support multiple -o options
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
` (4 preceding siblings ...)
2014-02-19 15:12 ` [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options Kevin Wolf
@ 2014-02-19 15:12 ` Kevin Wolf
2014-02-19 15:12 ` [Qemu-devel] [PATCH 7/7] qemu-img amend: " Kevin Wolf
2014-02-20 7:18 ` [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Markus Armbruster
7 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2014-02-19 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Multiple -o options has the same meaning as having a single option with
all settings in the order of their respective -o options.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 44 ++++++++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 30273ad..e0d19f8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1204,6 +1204,9 @@ static int img_convert(int argc, char **argv)
Error *local_err = NULL;
QemuOpts *sn_opts = NULL;
+ /* Initialize before goto out */
+ qemu_progress_init(progress, 1.0);
+
fmt = NULL;
out_fmt = NULL;
cache = NULL;
@@ -1223,21 +1226,24 @@ static int img_convert(int argc, char **argv)
case 'f':
if (fmt) {
error_report("-f may only be specified once");
- return 1;
+ ret = -1;
+ goto out;
}
fmt = optarg;
break;
case 'O':
if (out_fmt) {
error_report("-O may only be specified once");
- return 1;
+ ret = -1;
+ goto out;
}
out_fmt = optarg;
break;
case 'B':
if (out_baseimg) {
error_report("-B may only be specified once");
- return 1;
+ ret = -1;
+ goto out;
}
out_baseimg = optarg;
break;
@@ -1247,41 +1253,43 @@ static int img_convert(int argc, char **argv)
case 'e':
error_report("option -e is deprecated, please use \'-o "
"encryption\' instead!");
- return 1;
+ ret = -1;
+ goto out;
case '6':
error_report("option -6 is deprecated, please use \'-o "
"compat6\' instead!");
- return 1;
+ ret = -1;
+ goto out;
case 'o':
if (is_help_option(optarg)) {
options_help = true;
} else if (!options) {
- options = optarg;
+ options = g_strdup(optarg);
} else {
- error_report("-o cannot be used multiple times. Please use a "
- "single -o option with comma-separated settings "
- "instead.");
- return 1;
+ options = g_strdup_printf("%s,%s", options, optarg);
}
break;
case 's':
if (sn_opts || snapshot_name) {
error_report("-l/-s may only be specified once");
- return 1;
+ ret = -1;
+ goto out;
}
snapshot_name = optarg;
break;
case 'l':
if (sn_opts || snapshot_name) {
error_report("-l/-s may only be specified once");
- return 1;
+ ret = -1;
+ goto out;
}
if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
sn_opts = qemu_opts_parse(&internal_snapshot_opts, optarg, 0);
if (!sn_opts) {
error_report("Failed in parsing snapshot param '%s'",
optarg);
- return 1;
+ ret = -1;
+ goto out;
}
} else {
snapshot_name = optarg;
@@ -1294,7 +1302,8 @@ static int img_convert(int argc, char **argv)
sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);
if (sval < 0 || *end) {
error_report("Invalid minimum zero buffer size for sparse output specified");
- return 1;
+ ret = -1;
+ goto out;
}
min_sparse = sval / BDRV_SECTOR_SIZE;
@@ -1306,7 +1315,8 @@ static int img_convert(int argc, char **argv)
case 't':
if (cache) {
error_report("-t may only be specified once");
- return 1;
+ ret = -1;
+ goto out;
}
cache = optarg;
break;
@@ -1337,9 +1347,6 @@ static int img_convert(int argc, char **argv)
out_filename = argv[argc - 1];
- /* Initialize before goto out */
- qemu_progress_init(progress, 1.0);
-
if (options_help) {
ret = print_block_option_help(out_filename, out_fmt);
goto out;
@@ -1733,6 +1740,7 @@ out:
free_option_parameters(create_options);
free_option_parameters(param);
qemu_vfree(buf);
+ g_free(options);
if (sn_opts) {
qemu_opts_del(sn_opts);
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 7/7] qemu-img amend: Support multiple -o options
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
` (5 preceding siblings ...)
2014-02-19 15:12 ` [Qemu-devel] [PATCH 6/7] qemu-img convert: " Kevin Wolf
@ 2014-02-19 15:12 ` Kevin Wolf
2014-02-20 7:18 ` [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Markus Armbruster
7 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2014-02-19 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, mreitz
Multiple -o options has the same meaning as having a single option with
all settings in the order of their respective -o options.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index e0d19f8..1a98926 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2783,18 +2783,16 @@ static int img_amend(int argc, char **argv)
if (is_help_option(optarg)) {
options_help = true;
} else if (!options) {
- options = optarg;
+ options = g_strdup(optarg);
} else {
- error_report("-o cannot be used multiple times. Please use a "
- "single -o option with comma-separated settings "
- "instead.");
- return 1;
+ options = g_strdup_printf("%s,%s", options, optarg);
}
break;
case 'f':
if (fmt) {
error_report("-f may only be specified once");
- return 1;
+ ret = -1;
+ goto out;
}
fmt = optarg;
break;
@@ -2850,6 +2848,8 @@ out:
}
free_option_parameters(create_options);
free_option_parameters(options_param);
+ g_free(options);
+
if (ret) {
return 1;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options
2014-02-19 15:12 [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Kevin Wolf
` (6 preceding siblings ...)
2014-02-19 15:12 ` [Qemu-devel] [PATCH 7/7] qemu-img amend: " Kevin Wolf
@ 2014-02-20 7:18 ` Markus Armbruster
2014-02-20 9:03 ` Kevin Wolf
7 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2014-02-20 7:18 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> If you specify the same option more than once (e.g. -o cluster_size=4k
> -o lazy_refcounts=on), qemu-img silently ignores all but the last one. This
Sounds like perfectly common behavior to me.
> series fixes it to either consider all options or to give an error message.
Doesn't this break usage like "Compiled-in default is no good for me,
create wrapper script setting my defaults followed by "$@", then
override them on command line"?
> Boolean option can still be given more than once as they aren't problematic
> in this respect (ten times -q is still quiet).
Unless it's a toggle, but I sure hope we don't have those.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options
2014-02-20 7:18 ` [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options Markus Armbruster
@ 2014-02-20 9:03 ` Kevin Wolf
2014-02-20 9:48 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2014-02-20 9:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, stefanha, mreitz
Am 20.02.2014 um 08:18 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > If you specify the same option more than once (e.g. -o cluster_size=4k
> > -o lazy_refcounts=on), qemu-img silently ignores all but the last one. This
>
> Sounds like perfectly common behavior to me.
I guess it depends on the kind of option. For -o it's no less surprising
than 'gcc -Wall -Werror' giving you only -Werror without -Wall.
> > series fixes it to either consider all options or to give an error message.
>
> Doesn't this break usage like "Compiled-in default is no good for me,
> create wrapper script setting my defaults followed by "$@", then
> override them on command line"?
It does. Would you be happy with a series that accumulates -o options
(like this one does) and leaves the other options alone?
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] qemu-img: Fix handling of multiply specified options
2014-02-20 9:03 ` Kevin Wolf
@ 2014-02-20 9:48 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2014-02-20 9:48 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Am 20.02.2014 um 08:18 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > If you specify the same option more than once (e.g. -o cluster_size=4k
>> > -o lazy_refcounts=on), qemu-img silently ignores all but the last one. This
>>
>> Sounds like perfectly common behavior to me.
>
> I guess it depends on the kind of option. For -o it's no less surprising
> than 'gcc -Wall -Werror' giving you only -Werror without -Wall.
I agree that multiple -OPT KEY=VALUE,... accumulating is nicer than the
last one wiping out its predecessors.
>> > series fixes it to either consider all options or to give an error message.
>>
>> Doesn't this break usage like "Compiled-in default is no good for me,
>> create wrapper script setting my defaults followed by "$@", then
>> override them on command line"?
>
> It does. Would you be happy with a series that accumulates -o options
> (like this one does) and leaves the other options alone?
No objections.
^ permalink raw reply [flat|nested] 15+ messages in thread