[Qemu-devel] [PATCH v5 0/2] Clean up img_create() and introduce strtosz_suffix()

[Qemu-devel] [PATCH v5 0/2] Clean up img_create() and introduce strtosz_suffix()

[Jes.Sorensen]

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

This patch set introduces strtosz_suffix() which is needed to be able
to use strtosz parsing with a non MB default suffix. This is used to
clean up qemu-img.c:img_create().

Kevin asked me to rebase this instead of applying the other patches on
top, so please discard the previous versions. Sorry for the patch
noise.

v5 fixes the two issues pointed out by Stefan, making the call in
strtosz() explicitly use STRTOSZ_DEFSUFFIX_MB instead of 0 to specify
the default and adds a named argument to the prototype for
strtosz_suffix().

Jes Sorensen (2):
  Introduce strtosz_suffix()
  qemu-img.c: Clean up handling of image size in img_create()

 cutils.c      |   17 ++++++++++++++---
 qemu-common.h |    7 +++++++
 qemu-img.c    |   23 +++++++++++++++++------
 3 files changed, 38 insertions(+), 9 deletions(-)

-- 
1.7.3.2

[Qemu-devel] [PATCH 1/2] Introduce strtosz_suffix()

[Jes.Sorensen]

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

This introduces strtosz_suffix() which allows the caller to specify a
default suffix in case the non default of MB is wanted.

strtosz() is kept as a wrapper for strtosz_suffix() which keeps it's
current default of MB.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c      |   17 ++++++++++++++---
 qemu-common.h |    7 +++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/cutils.c b/cutils.c
index 28089aa..7984bc1 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,10 +291,10 @@ int fcntl_setfl(int fd, int flag)
  * value must be terminated by whitespace, ',' or '\0'. Return -1 on
  * error.
  */
-ssize_t strtosz(const char *nptr, char **end)
+ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
 {
     ssize_t retval = -1;
-    char *endptr, c;
+    char *endptr, c, d;
     int mul_required = 0;
     double val, mul, integral, fraction;
 
@@ -313,10 +313,16 @@ ssize_t strtosz(const char *nptr, char **end)
      * part of a multi token argument.
      */
     c = *endptr;
+    d = c;
     if (isspace(c) || c == '\0' || c == ',') {
         c = 0;
+        if (default_suffix) {
+            d = default_suffix;
+        } else {
+            d = c;
+        }
     }
-    switch (c) {
+    switch (d) {
     case 'B':
     case 'b':
         mul = 1;
@@ -371,3 +377,8 @@ fail:
 
     return retval;
 }
+
+ssize_t strtosz(const char *nptr, char **end)
+{
+    return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
+}
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..1ed32e5 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -149,7 +149,14 @@ time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
+
+#define STRTOSZ_DEFSUFFIX_TB	'T'
+#define STRTOSZ_DEFSUFFIX_GB	'G'
+#define STRTOSZ_DEFSUFFIX_MB	'M'
+#define STRTOSZ_DEFSUFFIX_KB	'K'
+#define STRTOSZ_DEFSUFFIX_B	'B'
 ssize_t strtosz(const char *nptr, char **end);
+ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
 
 /* path.c */
 void init_paths(const char *prefix);
-- 
1.7.3.2

Re: [Qemu-devel] [PATCH 1/2] Introduce strtosz_suffix()

[Markus Armbruster]

Sorry for the late review.

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> This introduces strtosz_suffix() which allows the caller to specify a
> default suffix in case the non default of MB is wanted.
>
> strtosz() is kept as a wrapper for strtosz_suffix() which keeps it's
> current default of MB.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  cutils.c      |   17 ++++++++++++++---
>  qemu-common.h |    7 +++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 28089aa..7984bc1 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -291,10 +291,10 @@ int fcntl_setfl(int fd, int flag)
>   * value must be terminated by whitespace, ',' or '\0'. Return -1 on
>   * error.
>   */
> -ssize_t strtosz(const char *nptr, char **end)
> +ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)

Last parameter's const is of marginal uility.  Matter of taste.

>  {
>      ssize_t retval = -1;
> -    char *endptr, c;
> +    char *endptr, c, d;
>      int mul_required = 0;
>      double val, mul, integral, fraction;
>  
> @@ -313,10 +313,16 @@ ssize_t strtosz(const char *nptr, char **end)
>       * part of a multi token argument.
>       */
>      c = *endptr;
> +    d = c;
>      if (isspace(c) || c == '\0' || c == ',') {
>          c = 0;
> +        if (default_suffix) {
> +            d = default_suffix;
> +        } else {
> +            d = c;
> +        }

The else clause "d = c" is effectively "d = 0" (due to prior "c = 0"),
which is the same as "d = default_suffix" (due to else's condition).
Thus, you can replace the conditional by a simple

    d = default_suffix;

>      }
> -    switch (c) {
> +    switch (d) {
>      case 'B':
>      case 'b':
>          mul = 1;
> @@ -371,3 +377,8 @@ fail:
>  
>      return retval;
>  }
> +
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> +    return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index de82c2e..1ed32e5 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -149,7 +149,14 @@ time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
> +
> +#define STRTOSZ_DEFSUFFIX_TB	'T'
> +#define STRTOSZ_DEFSUFFIX_GB	'G'
> +#define STRTOSZ_DEFSUFFIX_MB	'M'
> +#define STRTOSZ_DEFSUFFIX_KB	'K'
> +#define STRTOSZ_DEFSUFFIX_B	'B'

I don't see what these defines buy us over the literals, but if it makes
you or other reviewers happier...

>  ssize_t strtosz(const char *nptr, char **end);
> +ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
>  
>  /* path.c */
>  void init_paths(const char *prefix);

[Qemu-devel] [PATCH 2/2] 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 |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index d146d8c..f078718 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,20 @@ static int img_create(int argc, char **argv)
     }
     filename = argv[optind++];
 
+    /* Get image size, if specified */
+    if (optind < argc) {
+        ssize_t sval;
+        sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B);
+        if (sval < 0) {
+            error("Invalid image size specified! You may use k, M, G or "
+                  "T suffixes for ");
+            error("kilobytes, megabytes, gigabytes and terabytes.");
+            ret = -1;
+            goto out;
+        }
+        img_size = (uint64_t)sval;
+    }
+
     if (options && !strcmp(options, "?")) {
         ret = print_block_option_help(filename, fmt);
         goto out;
@@ -356,7 +371,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 +384,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

[Qemu-devel] Re: [PATCH v5 0/2] Clean up img_create() and introduce strtosz_suffix()

[Stefan Hajnoczi]

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH v5 0/2] Clean up img_create() and introduce strtosz_suffix()

[Kevin Wolf]

Am 09.12.2010 14:17, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This patch set introduces strtosz_suffix() which is needed to be able
> to use strtosz parsing with a non MB default suffix. This is used to
> clean up qemu-img.c:img_create().
> 
> Kevin asked me to rebase this instead of applying the other patches on
> top, so please discard the previous versions. Sorry for the patch
> noise.
> 
> v5 fixes the two issues pointed out by Stefan, making the call in
> strtosz() explicitly use STRTOSZ_DEFSUFFIX_MB instead of 0 to specify
> the default and adds a named argument to the prototype for
> strtosz_suffix().
> 
> Jes Sorensen (2):
>   Introduce strtosz_suffix()
>   qemu-img.c: Clean up handling of image size in img_create()
> 
>  cutils.c      |   17 ++++++++++++++---
>  qemu-common.h |    7 +++++++
>  qemu-img.c    |   23 +++++++++++++++++------
>  3 files changed, 38 insertions(+), 9 deletions(-)

Thanks, applied all to the block branch.

Kevin