diff mbox

[1/3] Consolidate printing of block driver options

Message ID 1291623456-3826-2-git-send-email-Jes.Sorensen@redhat.com
State New
Headers show

Commit Message

Jes Sorensen Dec. 6, 2010, 8:17 a.m. UTC 
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(-)

Comments

Stefan Hajnoczi Dec. 6, 2010, 9:32 a.m. UTC | #1 
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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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. UTC | #5 
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
diff mbox

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;
     }