Patchwork [1/3] Consolidate printing of block driver options

login
register
mail settings
Submitter Jes Sorensen
Date Dec. 2, 2010, 5:46 p.m.
Message ID <1291312009-24351-2-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/74006/
State New
Headers show

Comments

Jes Sorensen - Dec. 2, 2010, 5:46 p.m.
From: Jes Sorensen <Jes.Sorensen@redhat.com>

This consolidates the printing of block driver options in
print_block_option_help() which is called from both img_create() and
img_convert().

This allows for the "?" detection to be done just after the parsing of
options and the filename, instead of half way down the codepath of
these functions.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |   46 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 37 insertions(+), 9 deletions(-)
Stefan Hajnoczi - Dec. 3, 2010, 10:57 a.m.
On Thu, Dec 2, 2010 at 5:46 PM,  <Jes.Sorensen@redhat.com> wrote:
> @@ -188,6 +188,32 @@ static int read_password(char *buf, int buf_size)
>  }
>  #endif
>
> +static int print_block_option_help(const char *filename, const char *fmt)
> +{
> +    BlockDriver *drv, *proto_drv;
> +    QEMUOptionParameter *create_options = NULL;
> +
> +    /* Find driver and parse its options */
> +    drv = bdrv_find_format(fmt);
> +    if (!drv) {
> +        error("Unknown file format '%s'", fmt);
> +        return 1;
> +    }
> +
> +    proto_drv = bdrv_find_protocol(filename);
> +    if (!proto_drv) {
> +        error("Unknown protocol '%s'", filename);
> +        return 1;
> +    }
> +
> +    create_options = append_option_parameters(create_options,
> +                                              drv->create_options);
> +    create_options = append_option_parameters(create_options,
> +                                              proto_drv->create_options);
> +    print_option_help(create_options);

free_option_parameters(create_options);

> @@ -694,6 +720,11 @@ static int img_convert(int argc, char **argv)
>
>     out_filename = argv[argc - 1];
>
> +    if (options && !strcmp(options, "?")) {
> +        ret = print_block_option_help(out_filename, out_fmt);
> +        goto out2;
> +    }
> +
>     if (bs_n > 1 && out_baseimg) {
>         error("-B makes no sense when concatenating multiple input images");
>         return 1;
> @@ -749,10 +780,6 @@ static int img_convert(int argc, char **argv)
>                                               drv->create_options);
>     create_options = append_option_parameters(create_options,
>                                               proto_drv->create_options);
> -    if (options && !strcmp(options, "?")) {
> -        print_option_help(create_options);
> -        goto out;
> -    }
>
>     if (options) {
>         param = parse_option_parameters(options, create_options, param);
> @@ -984,6 +1011,7 @@ out:
>         }
>     }
>     free(bs);
> +out2:

Not needed, out is fine.  All those free functions are nops on NULL pointers.

Even simpler would be to just return early instead of jumping to a
label.  The other checks at the top of the function also do that.

Stefan
Jes Sorensen - Dec. 3, 2010, 10:59 a.m.
On 12/03/10 11:57, Stefan Hajnoczi wrote:
> On Thu, Dec 2, 2010 at 5:46 PM,  <Jes.Sorensen@redhat.com> wrote:
>> +    create_options = append_option_parameters(create_options,
>> +                                              drv->create_options);
>> +    create_options = append_option_parameters(create_options,
>> +                                              proto_drv->create_options);
>> +    print_option_help(create_options);
> 
> free_option_parameters(create_options);

Hmmm good point

>> @@ -694,6 +720,11 @@ static int img_convert(int argc, char **argv)
>>
>>     out_filename = argv[argc - 1];
>>
>> +    if (options && !strcmp(options, "?")) {
>> +        ret = print_block_option_help(out_filename, out_fmt);
>> +        goto out2;
>> +    }
>> +
>>     if (bs_n > 1 && out_baseimg) {
>>         error("-B makes no sense when concatenating multiple input images");
>>         return 1;
>> @@ -749,10 +780,6 @@ static int img_convert(int argc, char **argv)
>>                                               drv->create_options);
>>     create_options = append_option_parameters(create_options,
>>                                               proto_drv->create_options);
>> -    if (options && !strcmp(options, "?")) {
>> -        print_option_help(create_options);
>> -        goto out;
>> -    }
>>
>>     if (options) {
>>         param = parse_option_parameters(options, create_options, param);
>> @@ -984,6 +1011,7 @@ out:
>>         }
>>     }
>>     free(bs);
>> +out2:
> 
> Not needed, out is fine.  All those free functions are nops on NULL pointers.

Actually tried that, but it segfaulted, which is why I added out2.

Cheers,
Jes
Stefan Hajnoczi - Dec. 3, 2010, 10:59 a.m.
On Fri, Dec 3, 2010 at 10:57 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Dec 2, 2010 at 5:46 PM,  <Jes.Sorensen@redhat.com> wrote:
>> @@ -694,6 +720,11 @@ static int img_convert(int argc, char **argv)
>>
>>     out_filename = argv[argc - 1];
>>
>> +    if (options && !strcmp(options, "?")) {
>> +        ret = print_block_option_help(out_filename, out_fmt);
>> +        goto out2;
>> +    }
>> +
>>     if (bs_n > 1 && out_baseimg) {
>>         error("-B makes no sense when concatenating multiple input images");
>>         return 1;
>> @@ -749,10 +780,6 @@ static int img_convert(int argc, char **argv)
>>                                               drv->create_options);
>>     create_options = append_option_parameters(create_options,
>>                                               proto_drv->create_options);
>> -    if (options && !strcmp(options, "?")) {
>> -        print_option_help(create_options);
>> -        goto out;
>> -    }
>>
>>     if (options) {
>>         param = parse_option_parameters(options, create_options, param);
>> @@ -984,6 +1011,7 @@ out:
>>         }
>>     }
>>     free(bs);
>> +out2:
>
> Not needed, out is fine.  All those free functions are nops on NULL pointers.

Sorry, my comment is incorrect.  It is possible for bs_n > 0 and we'll
try to free bs[i].

Stefan

Patch

diff --git a/qemu-img.c b/qemu-img.c
index fa77ac0..99f30b3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,6 +188,32 @@  static int read_password(char *buf, int buf_size)
 }
 #endif
 
+static int print_block_option_help(const char *filename, const char *fmt)
+{
+    BlockDriver *drv, *proto_drv;
+    QEMUOptionParameter *create_options = NULL;
+
+    /* Find driver and parse its options */
+    drv = bdrv_find_format(fmt);
+    if (!drv) {
+        error("Unknown file format '%s'", fmt);
+        return 1;
+    }
+
+    proto_drv = bdrv_find_protocol(filename);
+    if (!proto_drv) {
+        error("Unknown protocol '%s'", filename);
+        return 1;
+    }
+
+    create_options = append_option_parameters(create_options,
+                                              drv->create_options);
+    create_options = append_option_parameters(create_options,
+                                              proto_drv->create_options);
+    print_option_help(create_options);
+    return 0;
+}
+
 static BlockDriverState *bdrv_new_open(const char *filename,
                                        const char *fmt,
                                        int flags)
@@ -310,6 +336,11 @@  static int img_create(int argc, char **argv)
         help();
     filename = argv[optind++];
 
+    if (options && !strcmp(options, "?")) {
+        ret = print_block_option_help(filename, fmt);
+        goto out;
+    }
+
     /* Find driver and parse its options */
     drv = bdrv_find_format(fmt);
     if (!drv) {
@@ -328,11 +359,6 @@  static int img_create(int argc, char **argv)
     create_options = append_option_parameters(create_options,
                                               proto_drv->create_options);
 
-    if (options && !strcmp(options, "?")) {
-        print_option_help(create_options);
-        goto out;
-    }
-
     /* Create parameter list with default values */
     param = parse_option_parameters("", create_options, param);
     set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
@@ -694,6 +720,11 @@  static int img_convert(int argc, char **argv)
 
     out_filename = argv[argc - 1];
 
+    if (options && !strcmp(options, "?")) {
+        ret = print_block_option_help(out_filename, out_fmt);
+        goto out2;
+    }
+
     if (bs_n > 1 && out_baseimg) {
         error("-B makes no sense when concatenating multiple input images");
         return 1;
@@ -749,10 +780,6 @@  static int img_convert(int argc, char **argv)
                                               drv->create_options);
     create_options = append_option_parameters(create_options,
                                               proto_drv->create_options);
-    if (options && !strcmp(options, "?")) {
-        print_option_help(create_options);
-        goto out;
-    }
 
     if (options) {
         param = parse_option_parameters(options, create_options, param);
@@ -984,6 +1011,7 @@  out:
         }
     }
     free(bs);
+out2:
     if (ret) {
         return 1;
     }