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

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

________________________________________________________________________________

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-v1:
https://github.com/jnsnow/qemu/releases/tag/qemu-io-tidy-v1

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 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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-io-cmds.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 6e5d1e4..07c5681 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,13 @@ 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 > INT_MAX) {
+        printf("signal argument '%s' is too large\n", argv[1]);
+        return 0;
     }
 
     /* Using raise() to kill this process does not necessarily flush all open
-- 
2.4.3

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

[Eric Blake]

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

On 10/26/2015 04:06 PM, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qemu-io-cmds.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 

> @@ -2191,10 +2190,13 @@ 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;

Pre-existing: attempting to raise signal -1 claims that -1 is
non-numeric.  Not the end of the world.

> +    } else if (sig > INT_MAX) {
> +        printf("signal argument '%s' is too large\n", argv[1]);
> +        return 0;
>      }

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

-- 
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 1/3] qemu-io: fix cvtnum lval types

[Eric Blake]

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

On 10/26/2015 04:40 PM, Eric Blake wrote:
> On 10/26/2015 04:06 PM, John Snow wrote:
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qemu-io-cmds.c | 30 ++++++++++++++++--------------
>>  1 file changed, 16 insertions(+), 14 deletions(-)
>>
> 
>> @@ -2191,10 +2190,13 @@ 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;
> 
> Pre-existing: attempting to raise signal -1 claims that -1 is
> non-numeric.  Not the end of the world.

I stand corrected; cvtnum() returns -ERANGE if the user attempted to
parse the string "-1" (that is, negative user input is not permitted, so
we always have a sane errno value).  [must be time for me to quit and
eat some dinner...]

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

And of course this still stands.

-- 
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 --]

[Qemu-devel] [PATCH 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>
---
 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 07c5681..e2477fc 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

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

[Eric Blake]

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

On 10/26/2015 04:06 PM, John Snow wrote:
> 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>
> ---
>  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 07c5681..e2477fc 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;
>  }

Eww.  This mixes up two return types, negative errno, and negative
input.  User input of -22 shouldn't behave differently than -21, just
because it happens to match -EINVAL.

Do we ever want to allow a negative return from cvtnum(), or should we
just blindly map a negative int64_t into -ERANGE for a contract that we
only accept 63-bit numbers?

-- 
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] [Qemu-block] [PATCH 2/3] qemu-io: Check for trailing chars

[Eric Blake]

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

On 10/26/2015 04:44 PM, Eric Blake wrote:
> On 10/26/2015 04:06 PM, John Snow wrote:
>> 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>
>> ---
>>  qemu-io-cmds.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)

>> +    ret = qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
>> +    if (*end != '\0') {
>> +        /* Detritus at the end of the string */
>> +        return -EINVAL;
>> +    }
>> +    return ret;
>>  }
> 
> Eww.  This mixes up two return types, negative errno, and negative
> input.  User input of -22 shouldn't behave differently than -21, just
> because it happens to match -EINVAL.
> 
> Do we ever want to allow a negative return from cvtnum(), or should we
> just blindly map a negative int64_t into -ERANGE for a contract that we
> only accept 63-bit numbers?

Uggh. Maybe I should read qemu_strtosz_suffix() before making bogus
claims (and assuming that it is merely sugar for strtoll).

I stand corrected - the only time you return negative values is if
qemu_strtosz_suffx() populated an errno.

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

-- 
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 --]

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

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-io-cmds.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e2477fc..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,10 +2214,11 @@ 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 > INT_MAX) {
-        printf("signal argument '%s' is too large\n", argv[1]);
+    } else if (sig > NSIG) {
+        printf("signal argument '%s' is too large to be a valid signal\n",
+               argv[1]);
         return 0;
     }
 
-- 
2.4.3

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

[Eric Blake]

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

On 10/26/2015 04:06 PM, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qemu-io-cmds.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index e2477fc..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);

s/ --/:/

> +        break;
> +    case -ERANGE:
> +        printf("Parsing error: argument too large -- %s\n", arg);
> +        break;
> +    default:
> +        printf("Parsing error -- %s\n", arg);

Twice more.

With that change,
Reviewed-by: Eric Blake <eblake@redhat.com>

> @@ -2199,10 +2214,11 @@ 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 > INT_MAX) {
> -        printf("signal argument '%s' is too large\n", argv[1]);
> +    } else if (sig > NSIG) {
> +        printf("signal argument '%s' is too large to be a valid signal\n",
> +               argv[1]);

Should the comparison against NSIG rather than INT_MAX be squashed into
patch 1?

-- 
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 3/3] qemu-io: Correct error messages

[John Snow]

On 10/26/2015 06:54 PM, Eric Blake wrote:
> On 10/26/2015 04:06 PM, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qemu-io-cmds.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 37 insertions(+), 21 deletions(-)
>>
>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>> index e2477fc..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);
> 
> s/ --/:/
> 
>> +        break;
>> +    case -ERANGE:
>> +        printf("Parsing error: argument too large -- %s\n", arg);
>> +        break;
>> +    default:
>> +        printf("Parsing error -- %s\n", arg);
> 
> Twice more.
> 
> With that change,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>> @@ -2199,10 +2214,11 @@ 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 > INT_MAX) {
>> -        printf("signal argument '%s' is too large\n", argv[1]);
>> +    } else if (sig > NSIG) {
>> +        printf("signal argument '%s' is too large to be a valid signal\n",
>> +               argv[1]);
> 
> Should the comparison against NSIG rather than INT_MAX be squashed into
> patch 1?
> 

Yes.