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

[Qemu-devel] [PATCH v4 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.

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..1d24d9a 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, 0);
+}
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..dc44cd6 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);
 
 /* path.c */
 void init_paths(const char *prefix);
-- 
1.7.3.2

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

[Stefan Hajnoczi]

On Thu, Dec 09, 2010 at 01:13:33PM +0100, Jes.Sorensen@redhat.com wrote:
> @@ -371,3 +377,8 @@ fail:
> 
>      return retval;
>  }
> +
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> +    return strtosz_suffix(nptr, end, 0);

This obscures what the default is, please use STRTOSZ_DEFSUFFIX_MB.  0
isn't very meaningful and requires the reader to dig into strtosz().

> diff --git a/qemu-common.h b/qemu-common.h
> index de82c2e..dc44cd6 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);

An argument name would be nice: const char unit?

Stefan

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

[Jes Sorensen]

On 12/09/10 13:53, Stefan Hajnoczi wrote:
> On Thu, Dec 09, 2010 at 01:13:33PM +0100, Jes.Sorensen@redhat.com wrote:
>> @@ -371,3 +377,8 @@ fail:
>>
>>      return retval;
>>  }
>> +
>> +ssize_t strtosz(const char *nptr, char **end)
>> +{
>> +    return strtosz_suffix(nptr, end, 0);
> 
> This obscures what the default is, please use STRTOSZ_DEFSUFFIX_MB.  0
> isn't very meaningful and requires the reader to dig into strtosz().

It doesn't make much different to me, but ok, that is easy to fix.

>> diff --git a/qemu-common.h b/qemu-common.h
>> index de82c2e..dc44cd6 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);
> 
> An argument name would be nice: const char unit?

I'll add that in the same round.

Cheers,
Jes

[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] [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 0/2] Fix size default for qemu-img

[Jes.Sorensen]

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

Kevin pointed out that my chance to img_create()'s handling of the
image size, changed the previous default of byte for size if no suffix
was specified, since strtosz() defaults to MB.

This patch set introduces strtosz_suffix() and then changes
img_create() to use that instead, thereby restoring the old default
behavior.

Jes Sorensen (2):
  Introduce strtosz_suffix()
  Make img_create() use strtosz_suffix()

 cutils.c      |   17 ++++++++++++++---
 qemu-common.h |    7 +++++++
 qemu-img.c    |    2 +-
 3 files changed, 22 insertions(+), 4 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..1d24d9a 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, 0);
+}
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..dc44cd6 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);
 
 /* path.c */
 void init_paths(const char *prefix);
-- 
1.7.3.2