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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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