[Qemu-devel] [PATCH v2 1/1] qemu-img.c: Clean up handling of image size in img_create()

[Qemu-devel] [PATCH v2 1/1] qemu-img.c: Clean up handling of image size in img_create()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This cleans up the handling of image size in img_create() by parsing
the value early, and then only setting it once if a value has been
added as the last argument to the command line.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index d146d8c..9986004 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -282,6 +282,7 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
 static int img_create(int argc, char **argv)
 {
     int c, ret = 0;
+    uint64_t img_size = -1;
     const char *fmt = "raw";
     const char *base_fmt = NULL;
     const char *filename;
@@ -329,6 +330,11 @@ static int img_create(int argc, char **argv)
     }
     filename = argv[optind++];
 
+    /* Get image size, if specified */
+    if (optind < argc) {
+        img_size = strtosz(argv[optind++], NULL);
+    }
+
     if (options && !strcmp(options, "?")) {
         ret = print_block_option_help(filename, fmt);
         goto out;
@@ -356,7 +362,8 @@ static int img_create(int argc, char **argv)
 
     /* Create parameter list with default values */
     param = parse_option_parameters("", create_options, param);
-    set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
+
+    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
 
     /* Parse -o options */
     if (options) {
@@ -368,11 +375,6 @@ static int img_create(int argc, char **argv)
         }
     }
 
-    /* Add size to parameters */
-    if (optind < argc) {
-        set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
-    }
-
     /* Add old-style options to parameters */
     ret = add_old_style_options(fmt, param, base_filename, base_fmt);
     if (ret < 0) {
-- 
1.7.3.2

Re: [Qemu-devel] [PATCH v2 1/1] qemu-img.c: Clean up handling of image size in img_create()

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> This cleans up the handling of image size in img_create() by parsing
> the value early, and then only setting it once if a value has been
> added as the last argument to the command line.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
>

Patch conflicts with commit c2abccec.

> diff --git a/qemu-img.c b/qemu-img.c
> index d146d8c..9986004 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -282,6 +282,7 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
>  static int img_create(int argc, char **argv)
>  {
>      int c, ret = 0;
> +    uint64_t img_size = -1;
>      const char *fmt = "raw";
>      const char *base_fmt = NULL;
>      const char *filename;
> @@ -329,6 +330,11 @@ static int img_create(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> +    /* Get image size, if specified */
> +    if (optind < argc) {
> +        img_size = strtosz(argv[optind++], NULL);

strtosz() can fail.  More below.

> +    }
> +
>      if (options && !strcmp(options, "?")) {
>          ret = print_block_option_help(filename, fmt);
>          goto out;
> @@ -356,7 +362,8 @@ static int img_create(int argc, char **argv)
>  
>      /* Create parameter list with default values */
>      param = parse_option_parameters("", create_options, param);
> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
> +
> +    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>  
>      /* Parse -o options */
>      if (options) {
> @@ -368,11 +375,6 @@ static int img_create(int argc, char **argv)
>          }
>      }
>  
> -    /* Add size to parameters */
> -    if (optind < argc) {
> -        set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
> -    }
> -
>      /* Add old-style options to parameters */
>      ret = add_old_style_options(fmt, param, base_filename, base_fmt);
>      if (ret < 0) {

This switches parsing of the size argument from parse_option_size() (via
set_option_parameter()) to strtosz().  I'm fine with that, but:

* Before:

    $ qemu-img create xxx xxx
    Parameter 'size' expects a size
    You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and terabytes.
    qemu-img: Image creation needs a size parameter

* After:

    $ qemu-img create xxx xxx
    qemu-img: Image creation needs a size parameter

Intentional?

Re: [Qemu-devel] [PATCH v2 1/1] qemu-img.c: Clean up handling of image size in img_create()

[Jes Sorensen]

On 12/15/10 17:47, Markus Armbruster wrote:
> Jes.Sorensen@redhat.com writes:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> This cleans up the handling of image size in img_create() by parsing
>> the value early, and then only setting it once if a value has been
>> added as the last argument to the command line.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  qemu-img.c |   14 ++++++++------
>>  1 files changed, 8 insertions(+), 6 deletions(-)
>>
> 
> Patch conflicts with commit c2abccec.

ARGH :(

> This switches parsing of the size argument from parse_option_size() (via
> set_option_parameter()) to strtosz().  I'm fine with that, but:
> 
> * Before:
> 
>     $ qemu-img create xxx xxx
>     Parameter 'size' expects a size
>     You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and terabytes.
>     qemu-img: Image creation needs a size parameter
> 
> * After:
> 
>     $ qemu-img create xxx xxx
>     qemu-img: Image creation needs a size parameter
> 
> Intentional?

This was addressed in the later revision when I introduced strtosz_suffix()

Cheers,
Jes

Re: [Qemu-devel] [PATCH v2 1/1] qemu-img.c: Clean up handling of image size in img_create()

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 12/15/10 17:47, Markus Armbruster wrote:
>> Jes.Sorensen@redhat.com writes:
>> 
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> This cleans up the handling of image size in img_create() by parsing
>>> the value early, and then only setting it once if a value has been
>>> added as the last argument to the command line.
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>>  qemu-img.c |   14 ++++++++------
>>>  1 files changed, 8 insertions(+), 6 deletions(-)
>>>
>> 
>> Patch conflicts with commit c2abccec.
>
> ARGH :(
>
>> This switches parsing of the size argument from parse_option_size() (via
>> set_option_parameter()) to strtosz().  I'm fine with that, but:
>> 
>> * Before:
>> 
>>     $ qemu-img create xxx xxx
>>     Parameter 'size' expects a size
>>     You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and terabytes.
>>     qemu-img: Image creation needs a size parameter
>> 
>> * After:
>> 
>>     $ qemu-img create xxx xxx
>>     qemu-img: Image creation needs a size parameter
>> 
>> Intentional?
>
> This was addressed in the later revision when I introduced strtosz_suffix()

Looks like I'm getting lost in my post-vacation mail backlog %-}