[Qemu-devel] [PATCH 0/2] qemu-img: fix coverity nits

[Qemu-devel] [PATCH 0/2] qemu-img: fix coverity nits

[Peter Maydell]

Coverity points out a couple of nits in qemu-img:
 * misuse of strtoul()
 * variable set but not used

These patches fix the issues.

Peter Maydell (2):
  qemu-img: Use qemu_strtoul() rather than raw strtoul()
  qemu-img: Avoid setting ret to unused value in img_convert()

 qemu-img.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] qemu-img: Use qemu_strtoul() rather than raw strtoul()

[Peter Maydell]

Some of the argument parsing in qemu-img uses strtoul() to parse
integer arguments.  This is tricky to get correct and in fact the
code does not get it right, because it assigns the result of
strtoul() to an 'int' variable and then tries to check for > INT_MAX.
Coverity correctly complains that the comparison is always false.

Rewrite to use qemu_strtoul(), which has a saner convention for
reporting conversion failures.

(Fixes CID 1356421, CID 1356422, CID 1356423.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-img.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 74e3362..aa71588 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3621,24 +3621,24 @@ static int img_bench(int argc, char **argv)
             break;
         case 'c':
         {
-            char *end;
-            errno = 0;
-            count = strtoul(optarg, &end, 0);
-            if (errno || *end || count > INT_MAX) {
+            unsigned long res;
+
+            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
                 error_report("Invalid request count specified");
                 return 1;
             }
+            count = res;
             break;
         }
         case 'd':
         {
-            char *end;
-            errno = 0;
-            depth = strtoul(optarg, &end, 0);
-            if (errno || *end || depth > INT_MAX) {
+            unsigned long res;
+
+            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
                 error_report("Invalid queue depth specified");
                 return 1;
             }
+            depth = res;
             break;
         }
         case 'f':
@@ -3705,24 +3705,24 @@ static int img_bench(int argc, char **argv)
             break;
         case OPTION_PATTERN:
         {
-            char *end;
-            errno = 0;
-            pattern = strtoul(optarg, &end, 0);
-            if (errno || *end || pattern > 0xff) {
+            unsigned long res;
+
+            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > 0xff) {
                 error_report("Invalid pattern byte specified");
                 return 1;
             }
+            pattern = res;
             break;
         }
         case OPTION_FLUSH_INTERVAL:
         {
-            char *end;
-            errno = 0;
-            flush_interval = strtoul(optarg, &end, 0);
-            if (errno || *end || flush_interval > INT_MAX) {
+            unsigned long res;
+
+            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
                 error_report("Invalid flush interval specified");
                 return 1;
             }
+            flush_interval = res;
             break;
         }
         case OPTION_NO_DRAIN:
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] qemu-img: Use qemu_strtoul() rather than raw strtoul()

[Philippe Mathieu-Daudé]

On 02/10/2017 01:28 PM, Peter Maydell wrote:
> Some of the argument parsing in qemu-img uses strtoul() to parse
> integer arguments.  This is tricky to get correct and in fact the
> code does not get it right, because it assigns the result of
> strtoul() to an 'int' variable and then tries to check for > INT_MAX.
> Coverity correctly complains that the comparison is always false.
>
> Rewrite to use qemu_strtoul(), which has a saner convention for
> reporting conversion failures.
>
> (Fixes CID 1356421, CID 1356422, CID 1356423.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  qemu-img.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 74e3362..aa71588 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3621,24 +3621,24 @@ static int img_bench(int argc, char **argv)
>              break;
>          case 'c':
>          {
> -            char *end;
> -            errno = 0;
> -            count = strtoul(optarg, &end, 0);
> -            if (errno || *end || count > INT_MAX) {
> +            unsigned long res;
> +
> +            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
>                  error_report("Invalid request count specified");
>                  return 1;
>              }
> +            count = res;
>              break;
>          }
>          case 'd':
>          {
> -            char *end;
> -            errno = 0;
> -            depth = strtoul(optarg, &end, 0);
> -            if (errno || *end || depth > INT_MAX) {
> +            unsigned long res;
> +
> +            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
>                  error_report("Invalid queue depth specified");
>                  return 1;
>              }
> +            depth = res;
>              break;
>          }
>          case 'f':
> @@ -3705,24 +3705,24 @@ static int img_bench(int argc, char **argv)
>              break;
>          case OPTION_PATTERN:
>          {
> -            char *end;
> -            errno = 0;
> -            pattern = strtoul(optarg, &end, 0);
> -            if (errno || *end || pattern > 0xff) {
> +            unsigned long res;
> +
> +            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > 0xff) {
>                  error_report("Invalid pattern byte specified");
>                  return 1;
>              }
> +            pattern = res;
>              break;
>          }
>          case OPTION_FLUSH_INTERVAL:
>          {
> -            char *end;
> -            errno = 0;
> -            flush_interval = strtoul(optarg, &end, 0);
> -            if (errno || *end || flush_interval > INT_MAX) {
> +            unsigned long res;
> +
> +            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
>                  error_report("Invalid flush interval specified");
>                  return 1;
>              }
> +            flush_interval = res;
>              break;
>          }
>          case OPTION_NO_DRAIN:
>

[Qemu-devel] [PATCH 2/2] qemu-img: Avoid setting ret to unused value in img_convert()

[Peter Maydell]

Coverity points out that we assign the return value from
bdrv_snapshot_load_tmp() to 'ret' in img_convert(), but then
never use that variable. (We check for failure by looking
at local_err instead.) Drop the unused assignment, bringing
the call into line with the following call to
bdrv_snapshot_laod_tmp_by_id_or_name().

(Fixes CID 1247240.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-img.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index aa71588..9227d3a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1966,10 +1966,10 @@ static int img_convert(int argc, char **argv)
     }
 
     if (sn_opts) {
-        ret = bdrv_snapshot_load_tmp(bs[0],
-                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
-                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
-                                     &local_err);
+        bdrv_snapshot_load_tmp(bs[0],
+                               qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
+                               qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
+                               &local_err);
     } else if (snapshot_name != NULL) {
         if (bs_n > 1) {
             error_report("No support for concatenating multiple snapshot");
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] qemu-img: Avoid setting ret to unused value in img_convert()

[Philippe Mathieu-Daudé]

On 02/10/2017 01:28 PM, Peter Maydell wrote:
> Coverity points out that we assign the return value from
> bdrv_snapshot_load_tmp() to 'ret' in img_convert(), but then
> never use that variable. (We check for failure by looking
> at local_err instead.) Drop the unused assignment, bringing
> the call into line with the following call to
> bdrv_snapshot_laod_tmp_by_id_or_name().
>
> (Fixes CID 1247240.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  qemu-img.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index aa71588..9227d3a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1966,10 +1966,10 @@ static int img_convert(int argc, char **argv)
>      }
>
>      if (sn_opts) {
> -        ret = bdrv_snapshot_load_tmp(bs[0],
> -                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
> -                                     qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
> -                                     &local_err);
> +        bdrv_snapshot_load_tmp(bs[0],
> +                               qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
> +                               qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
> +                               &local_err);
>      } else if (snapshot_name != NULL) {
>          if (bs_n > 1) {
>              error_report("No support for concatenating multiple snapshot");
>

Re: [Qemu-devel] [PATCH 0/2] qemu-img: fix coverity nits

[Max Reitz]

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

On 10.02.2017 17:28, Peter Maydell wrote:
> Coverity points out a couple of nits in qemu-img:
>  * misuse of strtoul()
>  * variable set but not used
> 
> These patches fix the issues.
> 
> Peter Maydell (2):
>   qemu-img: Use qemu_strtoul() rather than raw strtoul()
>   qemu-img: Avoid setting ret to unused value in img_convert()
> 
>  qemu-img.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)

Thanks, applied to my block tree:

https://github.com/XanClic/qemu/commits/block

Max


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