diff mbox

[20/24] qemu-img: Wrap cvtnum() around qemu_strtosz()

Message ID 1487067971-10443-21-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Feb. 14, 2017, 10:26 a.m. UTC 
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-img.c | 58 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

Comments

Eric Blake Feb. 17, 2017, 9:10 p.m. UTC | #1 
On 02/14/2017 04:26 AM, Markus Armbruster wrote:
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-img.c | 58 +++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 31 insertions(+), 27 deletions(-)


> @@ -3858,11 +3866,9 @@ static int img_dd_count(const char *arg,
>                          struct DdIo *in, struct DdIo *out,
>                          struct DdInfo *dd)
>  {
> -    char *end;
> +    dd->count = cvtnum(arg);

Hmm. cvtnum() accepts "1.5G", GNU dd does not. POSIX requires dd to
accept '1kx10k' to mean 10 mebibytes, and GNU dd accepts '10xM' as a
synonym, but cvtnum() does not accept all those corner cases.  POSIX
requires dd to treat '1b' as '512', while cvdnum() treats it as '1'.  I
sometimes wonder if our 'qemu-img dd' subcommand should be reusing the
numeric parsing that we use everywhere else, in spite of it meaning that
we are different than the POSIX quirks on what numbers are required to
be supported by dd.  But that's not the concern of this patch.

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster Feb. 18, 2017, 10:13 a.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 02/14/2017 04:26 AM, Markus Armbruster wrote:
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Cc: qemu-block@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qemu-img.c | 58 +++++++++++++++++++++++++++++++---------------------------
>>  1 file changed, 31 insertions(+), 27 deletions(-)
>
>
>> @@ -3858,11 +3866,9 @@ static int img_dd_count(const char *arg,
>>                          struct DdIo *in, struct DdIo *out,
>>                          struct DdInfo *dd)
>>  {
>> -    char *end;
>> +    dd->count = cvtnum(arg);
>
> Hmm. cvtnum() accepts "1.5G", GNU dd does not. POSIX requires dd to
> accept '1kx10k' to mean 10 mebibytes, and GNU dd accepts '10xM' as a
> synonym, but cvtnum() does not accept all those corner cases.  POSIX
> requires dd to treat '1b' as '512', while cvdnum() treats it as '1'.  I
> sometimes wonder if our 'qemu-img dd' subcommand should be reusing the
> numeric parsing that we use everywhere else, in spite of it meaning that
> we are different than the POSIX quirks on what numbers are required to
> be supported by dd.

qemu-img dd falls short of its stated goal to be like /usr/bin/dd.
Whether that's good or bad I decline to judge ;)

>                      But that's not the concern of this patch.

Yes.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
diff mbox

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 4407244..2a47966 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -368,6 +368,19 @@  static int add_old_style_options(const char *fmt, QemuOpts *opts,
     return 0;
 }
 
+static int64_t cvtnum(const char *s)
+{
+    char *end;
+    int64_t ret;
+
+    ret = qemu_strtosz(s, &end);
+    if (*end != '\0') {
+        /* Detritus at the end of the string */
+        return -EINVAL;
+    }
+    return ret;
+}
+
 static int img_create(int argc, char **argv)
 {
     int c;
@@ -461,9 +474,9 @@  static int img_create(int argc, char **argv)
     /* Get image size, if specified */
     if (optind < argc) {
         int64_t sval;
-        char *end;
-        sval = qemu_strtosz(argv[optind++], &end);
-        if (sval < 0 || *end) {
+
+        sval = cvtnum(argv[optind++]);
+        if (sval < 0) {
             if (sval == -ERANGE) {
                 error_report("Image size must be less than 8 EiB!");
             } else {
@@ -1861,9 +1874,9 @@  static int img_convert(int argc, char **argv)
         case 'S':
         {
             int64_t sval;
-            char *end;
-            sval = qemu_strtosz(optarg, &end);
-            if (sval < 0 || *end) {
+
+            sval = cvtnum(optarg);
+            if (sval < 0) {
                 error_report("Invalid minimum zero buffer size for sparse output specified");
                 ret = -1;
                 goto fail_getopt;
@@ -3648,10 +3661,8 @@  static int img_bench(int argc, char **argv)
             break;
         case 'o':
         {
-            char *end;
-            errno = 0;
-            offset = qemu_strtosz(optarg, &end);
-            if (offset < 0|| *end) {
+            offset = cvtnum(optarg);
+            if (offset < 0) {
                 error_report("Invalid offset specified");
                 return 1;
             }
@@ -3664,10 +3675,9 @@  static int img_bench(int argc, char **argv)
         case 's':
         {
             int64_t sval;
-            char *end;
 
-            sval = qemu_strtosz(optarg, &end);
-            if (sval < 0 || sval > INT_MAX || *end) {
+            sval = cvtnum(optarg);
+            if (sval < 0 || sval > INT_MAX) {
                 error_report("Invalid buffer size specified");
                 return 1;
             }
@@ -3678,10 +3688,9 @@  static int img_bench(int argc, char **argv)
         case 'S':
         {
             int64_t sval;
-            char *end;
 
-            sval = qemu_strtosz(optarg, &end);
-            if (sval < 0 || sval > INT_MAX || *end) {
+            sval = cvtnum(optarg);
+            if (sval < 0 || sval > INT_MAX) {
                 error_report("Invalid step size specified");
                 return 1;
             }
@@ -3840,12 +3849,11 @@  static int img_dd_bs(const char *arg,
                      struct DdIo *in, struct DdIo *out,
                      struct DdInfo *dd)
 {
-    char *end;
     int64_t res;
 
-    res = qemu_strtosz(arg, &end);
+    res = cvtnum(arg);
 
-    if (res <= 0 || res > INT_MAX || *end) {
+    if (res <= 0 || res > INT_MAX) {
         error_report("invalid number: '%s'", arg);
         return 1;
     }
@@ -3858,11 +3866,9 @@  static int img_dd_count(const char *arg,
                         struct DdIo *in, struct DdIo *out,
                         struct DdInfo *dd)
 {
-    char *end;
+    dd->count = cvtnum(arg);
 
-    dd->count = qemu_strtosz(arg, &end);
-
-    if (dd->count < 0 || *end) {
+    if (dd->count < 0) {
         error_report("invalid number: '%s'", arg);
         return 1;
     }
@@ -3892,11 +3898,9 @@  static int img_dd_skip(const char *arg,
                        struct DdIo *in, struct DdIo *out,
                        struct DdInfo *dd)
 {
-    char *end;
+    in->offset = cvtnum(arg);
 
-    in->offset = qemu_strtosz(arg, &end);
-
-    if (in->offset < 0 || *end) {
+    if (in->offset < 0) {
         error_report("invalid number: '%s'", arg);
         return 1;
     }