[Qemu-devel] [PATCH 1/1] Coverity: Fix failure path for qemu_accept in migration

[Qemu-devel] [PATCH 1/1] Coverity: Fix failure path for qemu_accept in migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Coverity defects 1005733 & 1005734 complain about passing a -ve value
to closesocket in the error paths on incoming migration.

Stash the error value and print it in the message (previously we gave
no indication of the reason for the failure)

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration-tcp.c  | 11 ++++++-----
 migration-unix.c | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 782572d..5c96cd3 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -56,19 +56,20 @@ static void tcp_accept_incoming_migration(void *opaque)
     socklen_t addrlen = sizeof(addr);
     int s = (intptr_t)opaque;
     QEMUFile *f;
-    int c;
+    int c, err;
 
     do {
         c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
-    } while (c == -1 && socket_error() == EINTR);
+        err = socket_error();
+    } while (c == -1 && err == EINTR);
     qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
     closesocket(s);
 
     DPRINTF("accepted migration\n");
 
-    if (c == -1) {
-        fprintf(stderr, "could not accept migration connection\n");
-        goto out;
+    if (c < 0) {
+        fprintf(stderr, "could not accept migration connection (%d)\n", err);
+        return;
     }
 
     f = qemu_fopen_socket(c, "rb");
diff --git a/migration-unix.c b/migration-unix.c
index 651fc5b..e7cf9a2 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -56,19 +56,20 @@ static void unix_accept_incoming_migration(void *opaque)
     socklen_t addrlen = sizeof(addr);
     int s = (intptr_t)opaque;
     QEMUFile *f;
-    int c;
+    int c, err;
 
     do {
         c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
-    } while (c == -1 && errno == EINTR);
+        err = errno;
+    } while (c == -1 && err == EINTR);
     qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
     close(s);
 
     DPRINTF("accepted migration\n");
 
-    if (c == -1) {
-        fprintf(stderr, "could not accept migration connection\n");
-        goto out;
+    if (c < 0) {
+        fprintf(stderr, "could not accept migration connection (%d)\n", err);
+        return;
     }
 
     f = qemu_fopen_socket(c, "rb");
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 1/1] Coverity: Fix failure path for qemu_accept in migration

[Peter Maydell]

On 19 March 2014 11:13, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Coverity defects 1005733 & 1005734 complain about passing a -ve value
> to closesocket in the error paths on incoming migration.
>
> Stash the error value and print it in the message (previously we gave
> no indication of the reason for the failure)
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration-tcp.c  | 11 ++++++-----
>  migration-unix.c | 11 ++++++-----
>  2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 782572d..5c96cd3 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -56,19 +56,20 @@ static void tcp_accept_incoming_migration(void *opaque)
>      socklen_t addrlen = sizeof(addr);
>      int s = (intptr_t)opaque;
>      QEMUFile *f;
> -    int c;
> +    int c, err;
>
>      do {
>          c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
> -    } while (c == -1 && socket_error() == EINTR);
> +        err = socket_error();
> +    } while (c == -1 && err == EINTR);
>      qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
>      closesocket(s);
>
>      DPRINTF("accepted migration\n");
>
> -    if (c == -1) {
> -        fprintf(stderr, "could not accept migration connection\n");
> -        goto out;
> +    if (c < 0) {

Why change the condition? Or alternatively, why use <0 here
but retain == -1 in the while condition above?

> +        fprintf(stderr, "could not accept migration connection (%d)\n", err);

Bit unfriendly not to convert the errno to a string.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/1] Coverity: Fix failure path for qemu_accept in migration

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 19 March 2014 11:13, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Coverity defects 1005733 & 1005734 complain about passing a -ve value
> > to closesocket in the error paths on incoming migration.
> >
> > Stash the error value and print it in the message (previously we gave
> > no indication of the reason for the failure)
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration-tcp.c  | 11 ++++++-----
> >  migration-unix.c | 11 ++++++-----
> >  2 files changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/migration-tcp.c b/migration-tcp.c
> > index 782572d..5c96cd3 100644
> > --- a/migration-tcp.c
> > +++ b/migration-tcp.c
> > @@ -56,19 +56,20 @@ static void tcp_accept_incoming_migration(void *opaque)
> >      socklen_t addrlen = sizeof(addr);
> >      int s = (intptr_t)opaque;
> >      QEMUFile *f;
> > -    int c;
> > +    int c, err;
> >
> >      do {
> >          c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
> > -    } while (c == -1 && socket_error() == EINTR);
> > +        err = socket_error();
> > +    } while (c == -1 && err == EINTR);
> >      qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
> >      closesocket(s);
> >
> >      DPRINTF("accepted migration\n");
> >
> > -    if (c == -1) {
> > -        fprintf(stderr, "could not accept migration connection\n");
> > -        goto out;
> > +    if (c < 0) {
> 
> Why change the condition? Or alternatively, why use <0 here
> but retain == -1 in the while condition above?

Because according to the manpage of accept(2) it's defined to return
-1 on error, or a +ve fd if it works, that while loop is purely checking
for the well defined case of EINTR i.e. -1/errno=EINTR; so the -1 in
the while loop is specific to the defined error case; I'm using < 0
here to catch -1 (which is what should happen) and anything undefined -
and thus make sure the close has a valid value.

> 
> > +        fprintf(stderr, "could not accept migration connection (%d)\n", err);
> 
> Bit unfriendly not to convert the errno to a string.

I could reroll it with a strerror.

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

Re: [Qemu-devel] [PATCH 1/1] Coverity: Fix failure path for qemu_accept in migration

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 19 March 2014 11:13, Dr. David Alan Gilbert (git)
>> <dgilbert@redhat.com> wrote:
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Coverity defects 1005733 & 1005734 complain about passing a -ve value
>> > to closesocket in the error paths on incoming migration.

What's a -ve value?  If you mean "negative", please spell it out.

>> > Stash the error value and print it in the message (previously we gave
>> > no indication of the reason for the failure)
>> >
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > ---
>> >  migration-tcp.c  | 11 ++++++-----
>> >  migration-unix.c | 11 ++++++-----
>> >  2 files changed, 12 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/migration-tcp.c b/migration-tcp.c
>> > index 782572d..5c96cd3 100644
>> > --- a/migration-tcp.c
>> > +++ b/migration-tcp.c
>> > @@ -56,19 +56,20 @@ static void tcp_accept_incoming_migration(void *opaque)
>> >      socklen_t addrlen = sizeof(addr);
>> >      int s = (intptr_t)opaque;
>> >      QEMUFile *f;
>> > -    int c;
>> > +    int c, err;
>> >
>> >      do {
>> >          c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
>> > -    } while (c == -1 && socket_error() == EINTR);
>> > +        err = socket_error();
>> > +    } while (c == -1 && err == EINTR);
>> >      qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
>> >      closesocket(s);
>> >
>> >      DPRINTF("accepted migration\n");
>> >
>> > -    if (c == -1) {
>> > -        fprintf(stderr, "could not accept migration connection\n");
>> > -        goto out;
>> > +    if (c < 0) {
>> 
>> Why change the condition? Or alternatively, why use <0 here
>> but retain == -1 in the while condition above?
>
> Because according to the manpage of accept(2) it's defined to return
> -1 on error, or a +ve fd if it works, that while loop is purely checking
> for the well defined case of EINTR i.e. -1/errno=EINTR; so the -1 in
> the while loop is specific to the defined error case; I'm using < 0
> here to catch -1 (which is what should happen) and anything undefined -
> and thus make sure the close has a valid value.

Some people use use < 0 to test for system call failure, some use == -1.
Both work.  Personally, I prefer < 0.  But I prefer locally consistent
usage even more.

>> > + fprintf(stderr, "could not accept migration connection (%d)\n",
>> > err);
>> 
>> Bit unfriendly not to convert the errno to a string.
>
> I could reroll it with a strerror.

Yes, please :)

Re: [Qemu-devel] [PATCH 1/1] Coverity: Fix failure path for qemu_accept in migration

[Paolo Bonzini]

Il 19/03/2014 12:34, Dr. David Alan Gilbert ha scritto:
>>> > > +        fprintf(stderr, "could not accept migration connection (%d)\n", err);
>> >
>> > Bit unfriendly not to convert the errno to a string.
> I could reroll it with a strerror.

Since you are at it, please use error_report too.

Paolo