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

login
register
mail settings
Submitter Jes Sorensen
Date Dec. 6, 2010, 8:17 a.m.
Message ID <1291623456-3826-2-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/74322/
State New
Headers show

Comments

Jes Sorensen - Dec. 6, 2010, 8:17 a.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 |   47 ++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 38 insertions(+), 9 deletions(-)
Stefan Hajnoczi - Dec. 6, 2010, 9:32 a.m.
On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
> @@ -694,6 +721,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;

Why goto out2 and not just return like the bs > 1 && out_baseimg check?

Stefan
Jes Sorensen - Dec. 6, 2010, 10:20 a.m.
On 12/06/10 10:32, Stefan Hajnoczi wrote:
> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>> @@ -694,6 +721,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;
> 
> Why goto out2 and not just return like the bs > 1 && out_baseimg check?

It is cleaner, I'd rather convert the bs_n test to do it too.

Cheers,
Jes
Stefan Hajnoczi - Dec. 6, 2010, 10:37 a.m.
On Mon, Dec 6, 2010 at 10:20 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 12/06/10 10:32, Stefan Hajnoczi wrote:
>> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>>> @@ -694,6 +721,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;
>>
>> Why goto out2 and not just return like the bs > 1 && out_baseimg check?
>
> It is cleaner, I'd rather convert the bs_n test to do it too.

"out2" tells me nothing and is just indirection for a return.  At this
point no resources have been acquired and it is simplest to bail out
early.

The segfault on out is fixed by setting up bs_n and bs[] together
instead of doing checks in between (that way we never have bs_n > 0
and bs == NULL).  Or by adding a check for bs != NULL to the out
cleanup code.  Then it would be safe to use out.

Stefan
Jes Sorensen - Dec. 6, 2010, 10:47 a.m.
On 12/06/10 11:37, Stefan Hajnoczi wrote:
> On Mon, Dec 6, 2010 at 10:20 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>> On 12/06/10 10:32, Stefan Hajnoczi wrote:
>>> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>>> Why goto out2 and not just return like the bs > 1 && out_baseimg check?
>>
>> It is cleaner, I'd rather convert the bs_n test to do it too.
> 
> "out2" tells me nothing and is just indirection for a return.  At this
> point no resources have been acquired and it is simplest to bail out
> early.

A consistent out path is more likely to catch issues if the code is
modified later. I am not a big fan of the random mix of return vs goto
out that we spray over the code.... or having help() call exit() for
that matter.

Cheers,
Jes
Stefan Hajnoczi - Dec. 6, 2010, 11:04 a.m.
On Mon, Dec 6, 2010 at 10:47 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 12/06/10 11:37, Stefan Hajnoczi wrote:
>> On Mon, Dec 6, 2010 at 10:20 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>>> On 12/06/10 10:32, Stefan Hajnoczi wrote:
>>>> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>>>> Why goto out2 and not just return like the bs > 1 && out_baseimg check?
>>>
>>> It is cleaner, I'd rather convert the bs_n test to do it too.
>>
>> "out2" tells me nothing and is just indirection for a return.  At this
>> point no resources have been acquired and it is simplest to bail out
>> early.
>
> A consistent out path is more likely to catch issues if the code is
> modified later. I am not a big fan of the random mix of return vs goto
> out that we spray over the code.... or having help() call exit() for
> that matter.

img_convert() wasn't random before your patch: return statements
before the first resource allocation, gotos afterwards. :P

I see what you're saying though.  How about making out work in all
cases and consistently using goto out?

Stefan

Patch

diff --git a/qemu-img.c b/qemu-img.c
index fa77ac0..7863835 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,6 +188,33 @@  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);
+    return 0;
+}
+
 static BlockDriverState *bdrv_new_open(const char *filename,
                                        const char *fmt,
                                        int flags)
@@ -310,6 +337,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 +360,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 +721,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 +781,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 +1012,7 @@  out:
         }
     }
     free(bs);
+out2:
     if (ret) {
         return 1;
     }