[Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options

2014-02-19 Thread Kevin Wolf 
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 kw...@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

Re: [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options

2014-02-19 Thread Fam Zheng 
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 kw...@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

Re: [Qemu-devel] [PATCH 5/7] qemu-img create: Support multiple -o options

2014-02-19 Thread Eric Blake 
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



signature.asc
Description: OpenPGP digital signature