[Qemu-devel] [PATCH 0/7] Six coverity fixes and a cleanup

[Qemu-devel] [PATCH 0/7] Six coverity fixes and a cleanup

[Paolo Bonzini]

Patch 2 is the cleanup.  The other six patches make the tcg, utils and
migration components clean.

Paolo Bonzini (7):
  cpu-exec: drop dead assignment
  cpu-exec: simplify icount code
  uri: avoid NULL arguments to strcmp
  qemu-sockets: improve error reporting in unix_listen_opts
  cutils: refine strtol error handling in parse_debug_env
  aes: remove a dead return statement
  migration: do floating-point division

 cpu-exec.c            | 12 +++---------
 migration/migration.c |  2 +-
 util/aes.c            |  2 +-
 util/cutils.c         |  4 ++--
 util/qemu-sockets.c   | 24 ++++++++++++++++++------
 util/uri.c            |  4 +++-
 6 files changed, 28 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/7] cpu-exec: drop dead assignment

[Paolo Bonzini]

All uses of TB inside cpu_exec are dominated by "tb = tb_find_fast(env)",
and there are no uses after the switch statement.  So the assignment
is dead, as reported by Coverity.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index a4f0eff..7aab86d 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -497,7 +497,6 @@ int cpu_exec(CPUArchState *env)
                          * interrupt_request) which we will handle
                          * next time around the loop.
                          */
-                        tb = (TranslationBlock *)(next_tb & ~TB_EXIT_MASK);
                         next_tb = 0;
                         break;
                     case TB_EXIT_ICOUNT_EXPIRED:
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] cpu-exec: simplify icount code

[Paolo Bonzini]

Use MIN instead of an "if" statement.  Move "tb" assignment where
the value is actually used.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 7aab86d..af56a34 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -502,22 +502,17 @@ int cpu_exec(CPUArchState *env)
                     case TB_EXIT_ICOUNT_EXPIRED:
                     {
                         /* Instruction counter expired.  */
-                        int insns_left;
-                        tb = (TranslationBlock *)(next_tb & ~TB_EXIT_MASK);
-                        insns_left = cpu->icount_decr.u32;
+                        int insns_left = cpu->icount_decr.u32;
                         if (cpu->icount_extra && insns_left >= 0) {
                             /* Refill decrementer and continue execution.  */
                             cpu->icount_extra += insns_left;
-                            if (cpu->icount_extra > 0xffff) {
-                                insns_left = 0xffff;
-                            } else {
-                                insns_left = cpu->icount_extra;
-                            }
+                            insns_left = MIN(0xffff, cpu->icount_extra);
                             cpu->icount_extra -= insns_left;
                             cpu->icount_decr.u16.low = insns_left;
                         } else {
                             if (insns_left > 0) {
                                 /* Execute remaining instructions.  */
+                                tb = (TranslationBlock *)(next_tb & ~TB_EXIT_MASK);
                                 cpu_exec_nocache(env, insns_left, tb);
                                 align_clocks(&sc, cpu);
                             }
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] uri: avoid NULL arguments to strcmp

[Paolo Bonzini]

Detected by Coverity.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/uri.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/uri.c b/util/uri.c
index 918d235..014d40d 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -1971,7 +1971,9 @@ uri_resolve_relative (const char *uri, const char * base)
 	val = g_strdup (uri);
 	goto done;
     }
-    if (!strcmp(bas->path, ref->path)) {
+    if (bas->path == ref->path ||
+        (bas->path != NULL && ref->path != NULL &&
+         !strcmp(bas->path, ref->path))) {
 	val = g_strdup("");
 	goto done;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] qemu-sockets: improve error reporting in unix_listen_opts

[Paolo Bonzini]

Coverity complains about not checking the returned value of mkstemp.  While
at it, also improve error checking for snprintf, and refine error messages
in general.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-sockets.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index a76bb3c..cf4b91f 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -694,7 +694,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
-        error_setg_errno(errp, errno, "Failed to create socket");
+        error_setg_errno(errp, errno, "Failed to create Unix socket");
         return -1;
     }
 
@@ -703,9 +703,15 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
     if (path && strlen(path)) {
         snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
     } else {
-        char *tmpdir = getenv("TMPDIR");
-        snprintf(un.sun_path, sizeof(un.sun_path), "%s/qemu-socket-XXXXXX",
-                 tmpdir ? tmpdir : "/tmp");
+        const char *tmpdir = getenv("TMPDIR");
+        tmpdir = tmpdir ? tmpdir : "/tmp";
+        if (snprintf(un.sun_path, sizeof(un.sun_path), "%s/qemu-socket-XXXXXX",
+                     tmpdir) >= sizeof(un.sun_path)) {
+            error_setg_errno(errp, errno,
+                             "TMPDIR environment variable (%s) too large", tmpdir);
+            goto err;
+        }
+
         /*
          * This dummy fd usage silences the mktemp() unsecure warning.
          * Using mkstemp() doesn't make things more secure here
@@ -713,13 +719,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
          * to unlink first and thus re-open the race window.  The
          * worst case possible is bind() failing, i.e. a DoS attack.
          */
-        fd = mkstemp(un.sun_path); close(fd);
+        fd = mkstemp(un.sun_path);
+        if (fd < 0) {
+            error_setg_errno(errp, errno,
+                             "Failed to make a temporary socket name in %s", tmpdir);
+            goto err;
+        }
+        close(fd);
         qemu_opt_set(opts, "path", un.sun_path);
     }
 
     unlink(un.sun_path);
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
-        error_setg_errno(errp, errno, "Failed to bind socket");
+        error_setg_errno(errp, errno, "Failed to bind socket to %s", un.sun_path);
         goto err;
     }
     if (listen(sock, 1) < 0) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] cutils: refine strtol error handling in parse_debug_env

[Paolo Bonzini]

Avoid truncation of a 64-bit long to a 32-bit int, and check for errno
(especially ERANGE).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/cutils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/cutils.c b/util/cutils.c
index dbe7412..f227064 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -523,7 +523,7 @@ int parse_debug_env(const char *name, int max, int initial)
 {
     char *debug_env = getenv(name);
     char *inv = NULL;
-    int debug;
+    long debug;
 
     if (!debug_env) {
         return initial;
@@ -532,7 +532,7 @@ int parse_debug_env(const char *name, int max, int initial)
     if (inv == debug_env) {
         return initial;
     }
-    if (debug < 0 || debug > max) {
+    if (debug < 0 || debug > max || errno != 0) {
         fprintf(stderr, "warning: %s not in [0, %d]", name, max);
         return initial;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-trivial] [PATCH 5/7] cutils: refine strtol error handling in parse_debug_env

[Michael Tokarev]

26.01.2015 14:12, Paolo Bonzini wrote:
> Avoid truncation of a 64-bit long to a 32-bit int, and check for errno
> (especially ERANGE).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/cutils.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index dbe7412..f227064 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -523,7 +523,7 @@ int parse_debug_env(const char *name, int max, int initial)
>  {
>      char *debug_env = getenv(name);
>      char *inv = NULL;
> -    int debug;
> +    long debug;
>  
>      if (!debug_env) {
>          return initial;
> @@ -532,7 +532,7 @@ int parse_debug_env(const char *name, int max, int initial)
>      if (inv == debug_env) {
>          return initial;
>      }
> -    if (debug < 0 || debug > max) {
> +    if (debug < 0 || debug > max || errno != 0) {

It is not really right to check errno without (re)setting it
before call to strtol().

Thanks,

/mjt

[Qemu-devel] [PATCH 6/7] aes: remove a dead return statement

[Paolo Bonzini]

bits is checked to be 128, 192 or 256 at the beginning of the function.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/aes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/aes.c b/util/aes.c
index 6058f19..f42126a 100644
--- a/util/aes.c
+++ b/util/aes.c
@@ -1161,7 +1161,7 @@ int AES_set_encrypt_key(const unsigned char *userKey, const int bits,
 			rk += 8;
         	}
 	}
-	return 0;
+	abort();
 }
 
 /**
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/7] migration: do floating-point division

[Paolo Bonzini]

Dividing integer expressions transferred_bytes and time_spent, and then converting
the integer quotient to type double. Any remainder, or fractional part of the
quotient, is ignored.  Fix this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index b3adbc6..6db75b8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -646,7 +646,7 @@ static void *migration_thread(void *opaque)
         if (current_time >= initial_time + BUFFER_DELAY) {
             uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
             uint64_t time_spent = current_time - initial_time;
-            double bandwidth = transferred_bytes / time_spent;
+            double bandwidth = (double)transferred_bytes / time_spent;
             max_size = bandwidth * migrate_max_downtime() / 1000000;
 
             s->mbps = time_spent ? (((double) transferred_bytes * 8.0) /
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 7/7] migration: do floating-point division

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Dividing integer expressions transferred_bytes and time_spent, and then converting
> the integer quotient to type double. Any remainder, or fractional part of the
> quotient, is ignored.  Fix this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b3adbc6..6db75b8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -646,7 +646,7 @@ static void *migration_thread(void *opaque)
>          if (current_time >= initial_time + BUFFER_DELAY) {
>              uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
>              uint64_t time_spent = current_time - initial_time;
> -            double bandwidth = transferred_bytes / time_spent;
> +            double bandwidth = (double)transferred_bytes / time_spent;
>              max_size = bandwidth * migrate_max_downtime() / 1000000;
>  
>              s->mbps = time_spent ? (((double) transferred_bytes * 8.0) /

This feels like it would be better to fix this by merging it into
the s->mbps calculation just off the bottom; we currently have:

            uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
            uint64_t time_spent = current_time - initial_time;
            double bandwidth = transferred_bytes / time_spent;
            max_size = bandwidth * migrate_max_downtime() / 1000000;

            s->mbps = time_spent ? (((double) transferred_bytes * 8.0) /
                    ((double) time_spent / 1000.0)) / 1000.0 / 1000.0 : -1;

 
Note that the mbps has a check for time_spent being 0 - if that can ever happen,
how come 'bandwidth' has never triggered it?
 
  transferred_bytes    -    bytes
  time_spent           -    ms
  bandwidth            -    bytes/ms
  migrate_max_downtime -    in ns
  s->mbps              -    mbit/s

giving

  max_size = bytes/ms * time-in-ns  / 1000000
           = bytes/ms * time-in-ms
           = bytes

so how about something like:
            uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
            uint64_t time_spent = current_time - initial_time;
            double   bytes_s    = (double) transferred_bytes / ((double) time_spent / 1000.0));
            s->mbps = (bytes_s * 8.0) / 1000000.0;
            max_size = bytes_s * (migrate_max_downtime() / 1000000000.0);

that also needs the trace fixing and the line a few lines below, I *think* we have
   dirty_bytes_rate is in bytes/second ? (arch_init.c)
   expected_downtime in ms ?
                s->expected_downtime = s->dirty_bytes_rate / bandwidth;
so,  bytes/s / bytes/ms    erm that's supposed to come out as time in ms

                s->expected_downtime = (int64_t)(1000 * (double)s->dirty_bytes_rate / bytes_s);

Yeuch.

Dave

> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 7/7] migration: do floating-point division

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> Dividing integer expressions transferred_bytes and time_spent, and then converting
>> the integer quotient to type double. Any remainder, or fractional part of the
>> quotient, is ignored.  Fix this.
>> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  migration/migration.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b3adbc6..6db75b8 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -646,7 +646,7 @@ static void *migration_thread(void *opaque)
>>          if (current_time >= initial_time + BUFFER_DELAY) {
>>              uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
>>              uint64_t time_spent = current_time - initial_time;
>> -            double bandwidth = transferred_bytes / time_spent;
>> +            double bandwidth = (double)transferred_bytes / time_spent;
>>              max_size = bandwidth * migrate_max_downtime() / 1000000;
>>  
>>              s->mbps = time_spent ? (((double) transferred_bytes * 8.0) /

Reviewed-by: Juan Quintela <quintela@redhat.com>

we had that fix on RHEL, but I forgot to push it upstream (it was not
needed as it optimized the calculations at the time).

>
> This feels like it would be better to fix this by merging it into
> the s->mbps calculation just off the bottom; we currently have:
>
>             uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
>             uint64_t time_spent = current_time - initial_time;
>             double bandwidth = transferred_bytes / time_spent;
>             max_size = bandwidth * migrate_max_downtime() / 1000000;
>
>             s->mbps = time_spent ? (((double) transferred_bytes * 8.0) /
>                     ((double) time_spent / 1000.0)) / 1000.0 / 1000.0 : -1;
>
>  
> Note that the mbps has a check for time_spent being 0 - if that can ever happen,
> how come 'bandwidth' has never triggered it?
>  
>   transferred_bytes    -    bytes
>   time_spent           -    ms
>   bandwidth            -    bytes/ms
>   migrate_max_downtime -    in ns
>   s->mbps              -    mbit/s
>
> giving
>
>   max_size = bytes/ms * time-in-ns  / 1000000
>            = bytes/ms * time-in-ms
>            = bytes
>
> so how about something like:
>             uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
>             uint64_t time_spent = current_time - initial_time;
>             double   bytes_s    = (double) transferred_bytes / ((double) time_spent / 1000.0));
>             s->mbps = (bytes_s * 8.0) / 1000000.0;
>             max_size = bytes_s * (migrate_max_downtime() / 1000000000.0);

I think we can do something like this on the normal tree, or just go for
sane units left and right?


> that also needs the trace fixing and the line a few lines below, I *think* we have
>    dirty_bytes_rate is in bytes/second ? (arch_init.c)
>    expected_downtime in ms ?
>                 s->expected_downtime = s->dirty_bytes_rate / bandwidth;
> so,  bytes/s / bytes/ms    erm that's supposed to come out as time in ms
>
>                 s->expected_downtime = (int64_t)(1000 * (double)s->dirty_bytes_rate / bytes_s);
>
> Yeuch.
>
> Dave
>
>> -- 
>> 1.8.3.1
>> 
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/7] Six coverity fixes and a cleanup

[Michael Tokarev]

26.01.2015 14:12, Paolo Bonzini wrote:
> Patch 2 is the cleanup.  The other six patches make the tcg, utils and
> migration components clean.

Applied to -trivial patches 1,2,3,4 and 6, not applied 5 (due to
questionable errno checking addition) and 7, due to a discussion.

Thanks,

/mjt

> 
> Paolo Bonzini (7):
>   cpu-exec: drop dead assignment
>   cpu-exec: simplify icount code
>   uri: avoid NULL arguments to strcmp
>   qemu-sockets: improve error reporting in unix_listen_opts
>   cutils: refine strtol error handling in parse_debug_env
>   aes: remove a dead return statement
>   migration: do floating-point division
> 
>  cpu-exec.c            | 12 +++---------
>  migration/migration.c |  2 +-
>  util/aes.c            |  2 +-
>  util/cutils.c         |  4 ++--
>  util/qemu-sockets.c   | 24 ++++++++++++++++++------
>  util/uri.c            |  4 +++-
>  6 files changed, 28 insertions(+), 20 deletions(-)
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/7] Six coverity fixes and a cleanup

[Paolo Bonzini]

On 07/02/2015 10:00, Michael Tokarev wrote:
>> > Patch 2 is the cleanup.  The other six patches make the tcg, utils and
>> > migration components clean.
> Applied to -trivial patches 1,2,3,4 and 6, not applied 5 (due to
> questionable errno checking addition) and 7, due to a discussion.

Thanks, will resubmit 5.

Juan/Amit, can you pick up 7 which Juan has already reviewed?

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/7] Six coverity fixes and a cleanup

[Michael Tokarev]

07.02.2015 23:01, Paolo Bonzini wrote:
> On 07/02/2015 10:00, Michael Tokarev wrote:
>>>> Patch 2 is the cleanup.  The other six patches make the tcg, utils and
>>>> migration components clean.
>> Applied to -trivial patches 1,2,3,4 and 6, not applied 5 (due to
>> questionable errno checking addition) and 7, due to a discussion.
> 
> Thanks, will resubmit 5.
> 
> Juan/Amit, can you pick up 7 which Juan has already reviewed?

I've no prob with 7, but please let's agree which way we're taking
there -- whenever subsequent fixes are needed, should be made on
the top of this change, or whole thing should be done differently.

Thanks.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/7] Six coverity fixes and a cleanup

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 07/02/2015 10:00, Michael Tokarev wrote:
>>> > Patch 2 is the cleanup.  The other six patches make the tcg, utils and
>>> > migration components clean.
>> Applied to -trivial patches 1,2,3,4 and 6, not applied 5 (due to
>> questionable errno checking addition) and 7, due to a discussion.
>
> Thanks, will resubmit 5.
>
> Juan/Amit, can you pick up 7 which Juan has already reviewed?

I pick 7.

Thanks, Juan.