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

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Peter Maydell (address@hidden) wrote:
>> On 19 March 2014 11:13, Dr. David Alan Gilbert (git)
>> <address@hidden> wrote:
>> > From: "Dr. David Alan Gilbert" <address@hidden>
>> >
>> > 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 <address@hidden>
>> > ---
>> >  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 :)