[Qemu-devel] [PATCH v2 0/3] qemu-io: clean up cvtnum usage

[Qemu-devel] [PATCH v2 0/3] qemu-io: clean up cvtnum usage

[John Snow]

cvtnum returns an int64_t, not an int, so correct the lvalue types
wherever it is used. While we're at it, make the error messages more
meaningful and hopefully less confusing.

v2:
 - Squashed NSIG error-checking from patch 3 into patch 1
 - Reported-by credits for Max and Reviewed-by from Eric added

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch qemu-io-tidy
https://github.com/jnsnow/qemu/tree/qemu-io-tidy

This version is tagged qemu-io-tidy-v2:
https://github.com/jnsnow/qemu/releases/tag/qemu-io-tidy-v2

John Snow (3):
  qemu-io: fix cvtnum lval types
  qemu-io: Check for trailing chars
  qemu-io: Correct error messages

 qemu-io-cmds.c | 93 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 34 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/3] qemu-io: fix cvtnum lval types

[John Snow]

cvtnum() returns int64_t: we should not be storing this
result inside of an int.

In a few cases, we need an extra sprinkling of error handling
where we expect to pass this number on towards a function that
expects something smaller than int64_t.

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-io-cmds.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 6e5d1e4..704db89 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -642,10 +642,11 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     int c, cnt;
     char *buf;
     int64_t offset;
-    int count;
+    int64_t count;
     /* Some compilers get confused and warn if this is not initialized.  */
     int total = 0;
-    int pattern = 0, pattern_offset = 0, pattern_count = 0;
+    int pattern = 0;
+    int64_t pattern_offset = 0, pattern_count = 0;
 
     while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
         switch (c) {
@@ -734,7 +735,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             return 0;
         }
         if (count & 0x1ff) {
-            printf("count %d is not sector aligned\n",
+            printf("count %"PRId64" is not sector aligned\n",
                    count);
             return 0;
         }
@@ -762,7 +763,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
         memset(cmp_buf, pattern, pattern_count);
         if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
             printf("Pattern verification failed at offset %"
-                   PRId64 ", %d bytes\n",
+                   PRId64 ", %"PRId64" bytes\n",
                    offset + pattern_offset, pattern_count);
         }
         g_free(cmp_buf);
@@ -957,7 +958,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
     int c, cnt;
     char *buf = NULL;
     int64_t offset;
-    int count;
+    int64_t count;
     /* Some compilers get confused and warn if this is not initialized.  */
     int total = 0;
     int pattern = 0xcd;
@@ -1029,7 +1030,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
         }
 
         if (count & 0x1ff) {
-            printf("count %d is not sector aligned\n",
+            printf("count %"PRId64" is not sector aligned\n",
                    count);
             return 0;
         }
@@ -1777,8 +1778,7 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
     struct timeval t1, t2;
     int Cflag = 0, qflag = 0;
     int c, ret;
-    int64_t offset;
-    int count;
+    int64_t offset, count;
 
     while ((c = getopt(argc, argv, "Cq")) != -1) {
         switch (c) {
@@ -1833,11 +1833,10 @@ out:
 static int alloc_f(BlockBackend *blk, int argc, char **argv)
 {
     BlockDriverState *bs = blk_bs(blk);
-    int64_t offset, sector_num;
-    int nb_sectors, remaining;
+    int64_t offset, sector_num, nb_sectors, remaining;
     char s1[64];
-    int num, sum_alloc;
-    int ret;
+    int num, ret;
+    int64_t sum_alloc;
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
@@ -1881,7 +1880,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
 
     cvtstr(offset, s1, sizeof(s1));
 
-    printf("%d/%d sectors allocated at offset %s\n",
+    printf("%"PRId64"/%"PRId64" sectors allocated at offset %s\n",
            sum_alloc, nb_sectors, s1);
     return 0;
 }
@@ -2191,10 +2190,14 @@ static const cmdinfo_t sigraise_cmd = {
 
 static int sigraise_f(BlockBackend *blk, int argc, char **argv)
 {
-    int sig = cvtnum(argv[1]);
+    int64_t sig = cvtnum(argv[1]);
     if (sig < 0) {
         printf("non-numeric signal number argument -- %s\n", argv[1]);
         return 0;
+    } else if (sig > NSIG) {
+        printf("signal argument '%s' is too large to be a valid signal\n",
+               argv[1]);
+        return 0;
     }
 
     /* Using raise() to kill this process does not necessarily flush all open
-- 
2.4.3

[Qemu-devel] [PATCH v2 2/3] qemu-io: Check for trailing chars

[John Snow]

Make sure there's not trailing garbage, e.g.
"64k-whatever-i-want-here"

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-io-cmds.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 704db89..44d24e8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -136,7 +136,14 @@ static char **breakline(char *input, int *count)
 static int64_t cvtnum(const char *s)
 {
     char *end;
-    return qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
+    int64_t ret;
+
+    ret = qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
+    if (*end != '\0') {
+        /* Detritus at the end of the string */
+        return -EINVAL;
+    }
+    return ret;
 }
 
 #define EXABYTES(x)     ((long long)(x) << 60)
-- 
2.4.3

[Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[John Snow]

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-io-cmds.c | 53 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 44d24e8..92c6b87 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
     return ret;
 }
 
+static void print_cvtnum_err(int64_t rc, const char *arg)
+{
+    switch (rc) {
+    case -EINVAL:
+        printf("Parsing error: non-numeric argument,"
+               " or extraneous/unrecognized suffix -- %s\n", arg);
+        break;
+    case -ERANGE:
+        printf("Parsing error: argument too large -- %s\n", arg);
+        break;
+    default:
+        printf("Parsing error -- %s\n", arg);
+    }
+}
+
 #define EXABYTES(x)     ((long long)(x) << 60)
 #define PETABYTES(x)    ((long long)(x) << 50)
 #define TERABYTES(x)    ((long long)(x) << 40)
@@ -366,13 +381,13 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov,
 
         len = cvtnum(arg);
         if (len < 0) {
-            printf("non-numeric length argument -- %s\n", arg);
+            print_cvtnum_err(len, arg);
             goto fail;
         }
 
         /* should be SIZE_T_MAX, but that doesn't exist */
         if (len > INT_MAX) {
-            printf("too large length argument -- %s\n", arg);
+            printf("Argument '%s' exceeds maximum size %d\n", arg, INT_MAX);
             goto fail;
         }
 
@@ -667,7 +682,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             lflag = 1;
             pattern_count = cvtnum(optarg);
             if (pattern_count < 0) {
-                printf("non-numeric length argument -- %s\n", optarg);
+                print_cvtnum_err(pattern_count, optarg);
                 return 0;
             }
             break;
@@ -688,7 +703,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             sflag = 1;
             pattern_offset = cvtnum(optarg);
             if (pattern_offset < 0) {
-                printf("non-numeric length argument -- %s\n", optarg);
+                print_cvtnum_err(pattern_offset, optarg);
                 return 0;
             }
             break;
@@ -711,14 +726,14 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     }
 
@@ -869,7 +884,7 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
     optind++;
@@ -1018,14 +1033,14 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     }
 
@@ -1150,7 +1165,7 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
     optind++;
@@ -1277,7 +1292,7 @@ static int multiwrite_f(BlockBackend *blk, int argc, char **argv)
         /* Read the offset of the request */
         offset = cvtnum(argv[optind]);
         if (offset < 0) {
-            printf("non-numeric offset argument -- %s\n", argv[optind]);
+            print_cvtnum_err(offset, argv[optind]);
             goto out;
         }
         optind++;
@@ -1504,7 +1519,7 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
 
     ctx->offset = cvtnum(argv[optind]);
     if (ctx->offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(ctx->offset, argv[optind]);
         g_free(ctx);
         return 0;
     }
@@ -1599,7 +1614,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
 
     ctx->offset = cvtnum(argv[optind]);
     if (ctx->offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(ctx->offset, argv[optind]);
         g_free(ctx);
         return 0;
     }
@@ -1659,7 +1674,7 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
-        printf("non-numeric truncate argument -- %s\n", argv[1]);
+        print_cvtnum_err(offset, argv[1]);
         return 0;
     }
 
@@ -1806,14 +1821,14 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     }
 
@@ -1847,7 +1862,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
-        printf("non-numeric offset argument -- %s\n", argv[1]);
+        print_cvtnum_err(offset, argv[1]);
         return 0;
     } else if (offset & 0x1ff) {
         printf("offset %" PRId64 " is not sector aligned\n",
@@ -1858,7 +1873,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
     if (argc == 3) {
         nb_sectors = cvtnum(argv[2]);
         if (nb_sectors < 0) {
-            printf("non-numeric length argument -- %s\n", argv[2]);
+            print_cvtnum_err(nb_sectors, argv[2]);
             return 0;
         }
     } else {
@@ -2199,7 +2214,7 @@ static int sigraise_f(BlockBackend *blk, int argc, char **argv)
 {
     int64_t sig = cvtnum(argv[1]);
     if (sig < 0) {
-        printf("non-numeric signal number argument -- %s\n", argv[1]);
+        print_cvtnum_err(sig, argv[1]);
         return 0;
     } else if (sig > NSIG) {
         printf("signal argument '%s' is too large to be a valid signal\n",
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[Eric Blake]

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

On 10/26/2015 05:45 PM, John Snow wrote:
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-io-cmds.c | 53 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 44d24e8..92c6b87 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
>      return ret;
>  }
>  
> +static void print_cvtnum_err(int64_t rc, const char *arg)
> +{
> +    switch (rc) {
> +    case -EINVAL:
> +        printf("Parsing error: non-numeric argument,"
> +               " or extraneous/unrecognized suffix -- %s\n", arg);
> +        break;
> +    case -ERANGE:
> +        printf("Parsing error: argument too large -- %s\n", arg);
> +        break;
> +    default:
> +        printf("Parsing error -- %s\n", arg);

I still think ':' is better than ' --' in error messages, but I'll leave
it up to the maintainer.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] qemu-io: fix cvtnum lval types

[Kevin Wolf]

Am 27.10.2015 um 00:45 hat John Snow geschrieben:
> cvtnum() returns int64_t: we should not be storing this
> result inside of an int.
> 
> In a few cases, we need an extra sprinkling of error handling
> where we expect to pass this number on towards a function that
> expects something smaller than int64_t.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-io-cmds.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 6e5d1e4..704db89 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -642,10 +642,11 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>      int c, cnt;
>      char *buf;
>      int64_t offset;
> -    int count;
> +    int64_t count;
>      /* Some compilers get confused and warn if this is not initialized.  */
>      int total = 0;
> -    int pattern = 0, pattern_offset = 0, pattern_count = 0;
> +    int pattern = 0;
> +    int64_t pattern_offset = 0, pattern_count = 0;
>  
>      while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
>          switch (c) {
> @@ -734,7 +735,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>              return 0;
>          }
>          if (count & 0x1ff) {
> -            printf("count %d is not sector aligned\n",
> +            printf("count %"PRId64" is not sector aligned\n",
>                     count);
>              return 0;
>          }
> @@ -762,7 +763,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>          memset(cmp_buf, pattern, pattern_count);
>          if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
>              printf("Pattern verification failed at offset %"
> -                   PRId64 ", %d bytes\n",
> +                   PRId64 ", %"PRId64" bytes\n",
>                     offset + pattern_offset, pattern_count);
>          }
>          g_free(cmp_buf);

read_f calls a few helper function which only take an int for count:

do_pread(), do_load_vmstate(), do_read() actually perform the request.
These should probably take int64_t as well (and if we want to be really
careful to avoid wraparounds, check limits individually).

qemu_io_alloc() takes size_t, so will wrap around on 32 bit hosts.
Should take int64_t and check against SIZE_MAX.

dump_buffer() also only takes an int, but I hope nobody tries to dump
more than 2 GB...

print_report() should probably be fixed to take int64_t.

And for total to make sense, it probably needs to be converted to
int64_t as well.

> @@ -957,7 +958,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>      int c, cnt;
>      char *buf = NULL;
>      int64_t offset;
> -    int count;
> +    int64_t count;
>      /* Some compilers get confused and warn if this is not initialized.  */
>      int total = 0;
>      int pattern = 0xcd;
> @@ -1029,7 +1030,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>          }
>  
>          if (count & 0x1ff) {
> -            printf("count %d is not sector aligned\n",
> +            printf("count %"PRId64" is not sector aligned\n",
>                     count);
>              return 0;
>          }

For writes, the helper functions to perform the request are different,
but they also only take int: do_pwrite(), do_save_vmstate(),
do_co_write_zeroes(), do_write_compressed(), do_write().

The rest should be fixed when you fix the helpers for read.

> @@ -1777,8 +1778,7 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
>      struct timeval t1, t2;
>      int Cflag = 0, qflag = 0;
>      int c, ret;
> -    int64_t offset;
> -    int count;
> +    int64_t offset, count;
>  
>      while ((c = getopt(argc, argv, "Cq")) != -1) {
>          switch (c) {

Here, blk_discard() is called directly without a helper function. A
check that the number of sectors fits in an int is missing.

> @@ -1833,11 +1833,10 @@ out:
>  static int alloc_f(BlockBackend *blk, int argc, char **argv)
>  {
>      BlockDriverState *bs = blk_bs(blk);
> -    int64_t offset, sector_num;
> -    int nb_sectors, remaining;
> +    int64_t offset, sector_num, nb_sectors, remaining;
>      char s1[64];
> -    int num, sum_alloc;
> -    int ret;
> +    int num, ret;
> +    int64_t sum_alloc;
>  
>      offset = cvtnum(argv[1]);
>      if (offset < 0) {
> @@ -1881,7 +1880,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
>  
>      cvtstr(offset, s1, sizeof(s1));
>  
> -    printf("%d/%d sectors allocated at offset %s\n",
> +    printf("%"PRId64"/%"PRId64" sectors allocated at offset %s\n",
>             sum_alloc, nb_sectors, s1);
>      return 0;
>  }

remaining is passed to bdrv_is_allocated() without checking against
INT_MAX first.

Kevin

Re: [Qemu-devel] [PATCH v2 2/3] qemu-io: Check for trailing chars

[Kevin Wolf]

Am 27.10.2015 um 00:45 hat John Snow geschrieben:
> Make sure there's not trailing garbage, e.g.
> "64k-whatever-i-want-here"
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[Kevin Wolf]

Am 27.10.2015 um 00:45 hat John Snow geschrieben:
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[Kevin Wolf]

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

Am 27.10.2015 um 03:26 hat Eric Blake geschrieben:
> On 10/26/2015 05:45 PM, John Snow wrote:
> > Reported-by: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  qemu-io-cmds.c | 53 ++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 34 insertions(+), 19 deletions(-)
> > 
> > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> > index 44d24e8..92c6b87 100644
> > --- a/qemu-io-cmds.c
> > +++ b/qemu-io-cmds.c
> > @@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
> >      return ret;
> >  }
> >  
> > +static void print_cvtnum_err(int64_t rc, const char *arg)
> > +{
> > +    switch (rc) {
> > +    case -EINVAL:
> > +        printf("Parsing error: non-numeric argument,"
> > +               " or extraneous/unrecognized suffix -- %s\n", arg);
> > +        break;
> > +    case -ERANGE:
> > +        printf("Parsing error: argument too large -- %s\n", arg);
> > +        break;
> > +    default:
> > +        printf("Parsing error -- %s\n", arg);
> 
> I still think ':' is better than ' --' in error messages, but I'll leave
> it up to the maintainer.

This isn't important enough for a maintainer decision - if this isn't
something that the patch submitter can decide by himself, what else
would be left? In particular because the patch only retains the existing
format. I'm happy to merge a patch that uses colons instead, but I won't
reject anything just because it doesn't do the conversion.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[John Snow]

On 10/26/2015 10:26 PM, Eric Blake wrote:
> On 10/26/2015 05:45 PM, John Snow wrote:
>> Reported-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  qemu-io-cmds.c | 53 ++++++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>> index 44d24e8..92c6b87 100644
>> --- a/qemu-io-cmds.c
>> +++ b/qemu-io-cmds.c
>> @@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
>>      return ret;
>>  }
>>  
>> +static void print_cvtnum_err(int64_t rc, const char *arg)
>> +{
>> +    switch (rc) {
>> +    case -EINVAL:
>> +        printf("Parsing error: non-numeric argument,"
>> +               " or extraneous/unrecognized suffix -- %s\n", arg);
>> +        break;
>> +    case -ERANGE:
>> +        printf("Parsing error: argument too large -- %s\n", arg);
>> +        break;
>> +    default:
>> +        printf("Parsing error -- %s\n", arg);
> 
> I still think ':' is better than ' --' in error messages, but I'll leave
> it up to the maintainer.
> 

Crud, sorry Eric -- I didn't do this on purpose. As Kevin notes, I was
just trying to match the existing format. I can change it and send again
if you want. Whatever is easiest for people.

--js

Re: [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[Kevin Wolf]

Am 27.10.2015 um 16:50 hat John Snow geschrieben:
> 
> 
> On 10/26/2015 10:26 PM, Eric Blake wrote:
> > On 10/26/2015 05:45 PM, John Snow wrote:
> >> Reported-by: Max Reitz <mreitz@redhat.com>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  qemu-io-cmds.c | 53 ++++++++++++++++++++++++++++++++++-------------------
> >>  1 file changed, 34 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> >> index 44d24e8..92c6b87 100644
> >> --- a/qemu-io-cmds.c
> >> +++ b/qemu-io-cmds.c
> >> @@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
> >>      return ret;
> >>  }
> >>  
> >> +static void print_cvtnum_err(int64_t rc, const char *arg)
> >> +{
> >> +    switch (rc) {
> >> +    case -EINVAL:
> >> +        printf("Parsing error: non-numeric argument,"
> >> +               " or extraneous/unrecognized suffix -- %s\n", arg);
> >> +        break;
> >> +    case -ERANGE:
> >> +        printf("Parsing error: argument too large -- %s\n", arg);
> >> +        break;
> >> +    default:
> >> +        printf("Parsing error -- %s\n", arg);
> > 
> > I still think ':' is better than ' --' in error messages, but I'll leave
> > it up to the maintainer.
> 
> Crud, sorry Eric -- I didn't do this on purpose. As Kevin notes, I was
> just trying to match the existing format. I can change it and send again
> if you want. Whatever is easiest for people.

I think you need to respin for patch 1 anyway, so changing it in the
next version sounds good. You can keep my R-b when doing this.

Kevin

Re: [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[Eric Blake]

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

On 10/27/2015 09:50 AM, John Snow wrote:

>>> +    default:
>>> +        printf("Parsing error -- %s\n", arg);
>>
>> I still think ':' is better than ' --' in error messages, but I'll leave
>> it up to the maintainer.
>>
> 
> Crud, sorry Eric -- I didn't do this on purpose. As Kevin notes, I was
> just trying to match the existing format. I can change it and send again
> if you want. Whatever is easiest for people.

And Kevin has a valid point that you just did code motion, so keeping --
is no worse than before.  At this point, I'll leave it up to you; my R-b
stands either way.

-- 
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: 604 bytes --]