From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41244) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQEle-0000w6-GH for qemu-devel@nongnu.org; Wed, 19 Mar 2014 07:34:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQElZ-0003Dh-MD for qemu-devel@nongnu.org; Wed, 19 Mar 2014 07:34:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26058) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQElZ-0003Ar-Do for qemu-devel@nongnu.org; Wed, 19 Mar 2014 07:34:37 -0400 Date: Wed, 19 Mar 2014 11:34:30 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20140319113429.GG2371@work-vm> References: <1395227624-20725-1-git-send-email-dgilbert@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1/1] Coverity: Fix failure path for qemu_accept in migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , Juan Quintela * Peter Maydell (peter.maydell@linaro.org) wrote: > On 19 March 2014 11:13, Dr. David Alan Gilbert (git) > wrote: > > From: "Dr. David Alan Gilbert" > > > > 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 > > --- > > 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