| Message ID | 1395227624-20725-1-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
* 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
"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 :)
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
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");