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

[Markus Armbruster]

address@hidden writes:

> From: Jes Sorensen <address@hidden>
>
> 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 <address@hidden>
> ---
>  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?