[Qemu-devel] [PATCH v3] qemu-img: correct size parsers and help message

[Qemu-devel] [PATCH v3] qemu-img: correct size parsers and help message

[Dong Xu Wang]

qemu-img not only suports k/K/M/G/T/b, but also supports m/g/t/B. So correct
it in help message.

Also use the same parser in parse_option_size function.

Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
CC: riegamaths@gmail.com
---
v1->v2: also correct error reporting.
v2->v3: use the same parser in parse_option_size function.

 qemu-img.c    |    9 +++++----
 qemu-option.c |    6 +++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 80cfb9b..7f2fde4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -69,8 +69,9 @@ static void help(void)
            "    options are: 'none', 'writeback' (default, except for convert), 'writethrough',\n"
            "    'directsync' and 'unsafe' (default for convert)\n"
            "  'size' is the disk image size in bytes. Optional suffixes\n"
-           "    'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M)\n"
-           "    and T (terabyte, 1024G) are supported. 'b' is ignored.\n"
+           "    'k' or 'K' (kilobyte, 1024), 'm' or 'M' (megabyte, 1024k),\n"
+           "    'g' or 'G' (gigabyte, 1024M) and 't' or 'T' (terabyte, 1024G) are supported.\n"
+           "    'b' or 'B' is ignored.\n"
            "  'output_filename' is the destination disk image filename\n"
            "  'output_fmt' is the destination format\n"
            "  'options' is a comma separated list of format specific options in a\n"
@@ -341,8 +342,8 @@ static int img_create(int argc, char **argv)
         char *end;
         sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
         if (sval < 0 || *end) {
-            error_report("Invalid image size specified! You may use k, M, G or "
-                  "T suffixes for ");
+            error_report("Invalid image size specified! You may use k/K, m/M, "
+                  "g/G or t/T suffixes for ");
             error_report("kilobytes, megabytes, gigabytes and terabytes.");
             ret = -1;
             goto out;
diff --git a/qemu-option.c b/qemu-option.c
index bb3886c..abc3b90 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -213,25 +213,29 @@ static void parse_option_size(const char *name, const char *value,
         sizef = strtod(value, &postfix);
         switch (*postfix) {
         case 'T':
+        case 't':
             sizef *= 1024;
             /* fall through */
         case 'G':
+        case 'g':
             sizef *= 1024;
             /* fall through */
         case 'M':
+        case 'm':
             sizef *= 1024;
             /* fall through */
         case 'K':
         case 'k':
             sizef *= 1024;
             /* fall through */
+        case 'B':
         case 'b':
         case '\0':
             *ret = (uint64_t) sizef;
             break;
         default:
             error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
-            error_printf_unless_qmp("You may use k, M, G or T suffixes for "
+            error_printf_unless_qmp("You may use K/k, M/m, G/g or T/t suffixes for "
                     "kilobytes, megabytes, gigabytes and terabytes.\n");
             return;
         }
-- 
1.7.1

Re: [Qemu-devel] [PATCH v3] qemu-img: correct size parsers and help message

[Kevin Wolf]

Am 18.07.2012 15:23, schrieb Dong Xu Wang:
> qemu-img not only suports k/K/M/G/T/b, but also supports m/g/t/B. So correct
> it in help message.
> 
> Also use the same parser in parse_option_size function.

This is not what the patch does. It uses a parser that seems slightly
more compatible with strtosz_suffix() than before, but it still doesn't
use the same one. It really should call strtosz_suffix() instead of
implementing a parser here.

Kevin

Re: [Qemu-devel] [PATCH v3] qemu-img: correct size parsers and help message

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

On 07/18/2012 07:23 AM, Dong Xu Wang wrote:
> qemu-img not only suports k/K/M/G/T/b, but also supports m/g/t/B. So correct
> it in help message.
> 

> +++ b/qemu-img.c
> @@ -69,8 +69,9 @@ static void help(void)
>             "    options are: 'none', 'writeback' (default, except for convert), 'writethrough',\n"
>             "    'directsync' and 'unsafe' (default for convert)\n"
>             "  'size' is the disk image size in bytes. Optional suffixes\n"
> -           "    'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M)\n"
> -           "    and T (terabyte, 1024G) are supported. 'b' is ignored.\n"
> +           "    'k' or 'K' (kilobyte, 1024), 'm' or 'M' (megabyte, 1024k),\n"
> +           "    'g' or 'G' (gigabyte, 1024M) and 't' or 'T' (terabyte, 1024G) are supported.\n"
> +           "    'b' or 'B' is ignored.\n"

Technically, 'kilobyte' is only 1000 bytes; the correct term for 1024
bytes is 'kibibyte'.  Likewise for 'megabyte' (1000000) vs. 'mebibyte'
(1024k, or 1,048,576 bytes); and so on for gibibytes and tebibytes.
Since disk manufacturers have already forced the rest of the world to
ask whether the number of bytes they are looking at is a power of 10 or
a power of 2 suffix, we might as well be precise in our naming to
document that we really are using powers of 2.

Furthermore, I think you can compress this by mentioning that the parse
is case-insensitive, instead of spelling out all the options:

'size' is the disk image size in bytes, scaled by an optional
case-insensitive suffix: 'k' (kibibyte, 1024), 'M' (mebibyte, 1024k),
'G' (gibibyte, 1024M), 'T' (tebibyte, 1024G), or 'b' (no scaling).

> @@ -341,8 +342,8 @@ static int img_create(int argc, char **argv)
>          char *end;
>          sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
>          if (sval < 0 || *end) {
> -            error_report("Invalid image size specified! You may use k, M, G or "
> -                  "T suffixes for ");
> +            error_report("Invalid image size specified! You may use k/K, m/M, "
> +                  "g/G or t/T suffixes for ");

I personally dislike this change.  Just because we're lenient in what we
accept does not mean we have to document all of the possibilities that
we parse when correcting a user error; rather, we need only document the
preferred possibilities.

>          default:
>              error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
> -            error_printf_unless_qmp("You may use k, M, G or T suffixes for "
> +            error_printf_unless_qmp("You may use K/k, M/m, G/g or T/t suffixes for "

Again, in an error message, I'd only document the preferred capitalization.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]