| Message ID | 1343811543-12137-2-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 01, 2012 at 04:59:01PM +0800, Amos Kong wrote: > It's aleady in the end of loop, error should be set. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > qemu-sockets.c | 4 +--- > 1 files changed, 1 insertions(+), 3 deletions(-) > > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 668fa93..c636882 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -181,9 +181,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp) > fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__, > inet_strfamily(e->ai_family), uaddr, inet_getport(e), > strerror(errno)); > - if (!e->ai_next) { > - error_set(errp, QERR_SOCKET_BIND_FAILED); > - } > + error_set(errp, QERR_SOCKET_BIND_FAILED); > } > } > closesocket(slisten); This isn't obvious. It looks like the intent of the if (!e->ai_next) is to suppress the error so that the next iteration of the *outer* loop can succeed. Why is it okay to set QERR_SOCKET_BIND_FAILED? We may have more addrinfos left to try in the outer loop. They may succeed so we don't want an error in that case. Stefan
On 1 August 2012 11:50, Stefan Hajnoczi <stefanha@gmail.com> wrote: > This isn't obvious. It looks like the intent of the if (!e->ai_next) is > to suppress the error so that the next iteration of the *outer* loop can > succeed. Yeah, we only call it an error on the last time round. This loop is a bit confusingly structured, since we're effectively handling the failure in several places at once: we always fprintf() something, then we set the error on the last time round the loop, then at the end of the loop we fprintf again. We also duplicate the loop termination condition. It might be better to have an Error *local_err in this function. Then we could unconditionally call error_set() for any failures, passing &local_err. Then at the end of the loop we can call error_propagate(errp, local_err) to pass an error up if we didn't succeed at all. Unfortunately you'd have to do if (error_is_set(&local_err)) { error_free(&local_err); } error_set(&local_err, QERR_whatever); for the error setting, since error_set() will assert if you try to set an error twice. [Another demonstration of the fprintf errors being much more useful than the error_set mechanisms at the moment, incidentally. We can get away with the fprintfs because the only caller of this function which passes in a non-NULL errp is the migration code called from vl.c, which is just going to fprintf and exit on error anyway.] -- PMM
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Wed, Aug 01, 2012 at 04:59:01PM +0800, Amos Kong wrote: >> It's aleady in the end of loop, error should be set. >> >> Signed-off-by: Amos Kong <akong@redhat.com> >> --- >> qemu-sockets.c | 4 +--- >> 1 files changed, 1 insertions(+), 3 deletions(-) >> >> diff --git a/qemu-sockets.c b/qemu-sockets.c >> index 668fa93..c636882 100644 >> --- a/qemu-sockets.c >> +++ b/qemu-sockets.c >> @@ -181,9 +181,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp) >> fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__, >> inet_strfamily(e->ai_family), uaddr, inet_getport(e), >> strerror(errno)); >> - if (!e->ai_next) { >> - error_set(errp, QERR_SOCKET_BIND_FAILED); >> - } >> + error_set(errp, QERR_SOCKET_BIND_FAILED); >> } >> } >> closesocket(slisten); > > This isn't obvious. It looks like the intent of the if (!e->ai_next) is > to suppress the error so that the next iteration of the *outer* loop can > succeed. > > Why is it okay to set QERR_SOCKET_BIND_FAILED? We may have more > addrinfos left to try in the outer loop. They may succeed so we don't > want an error in that case. You are correct, and the patch is wrong. See also related http://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg00772.html
Peter Maydell <peter.maydell@linaro.org> writes: > On 1 August 2012 11:50, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> This isn't obvious. It looks like the intent of the if (!e->ai_next) is >> to suppress the error so that the next iteration of the *outer* loop can >> succeed. > > Yeah, we only call it an error on the last time round. This > loop is a bit confusingly structured, since we're effectively > handling the failure in several places at once: we always > fprintf() something, then we set the error on the last time > round the loop, then at the end of the loop we fprintf again. > We also duplicate the loop termination condition. I cleaned it up some back in February, but my admittedly incremental improvement was rejected because it didn't also convert to error_set(). *shrug* http://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg00772.html [...]
----- Original Message ----- > Stefan Hajnoczi <stefanha@gmail.com> writes: > > > On Wed, Aug 01, 2012 at 04:59:01PM +0800, Amos Kong wrote: > >> It's aleady in the end of loop, error should be set. > >> > >> Signed-off-by: Amos Kong <akong@redhat.com> > >> --- > >> qemu-sockets.c | 4 +--- > >> 1 files changed, 1 insertions(+), 3 deletions(-) > >> > >> diff --git a/qemu-sockets.c b/qemu-sockets.c > >> index 668fa93..c636882 100644 > >> --- a/qemu-sockets.c > >> +++ b/qemu-sockets.c > >> @@ -181,9 +181,7 @@ int inet_listen_opts(QemuOpts *opts, int > >> port_offset, Error **errp) > >> fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", > >> __FUNCTION__, > >> inet_strfamily(e->ai_family), uaddr, > >> inet_getport(e), > >> strerror(errno)); > >> - if (!e->ai_next) { > >> - error_set(errp, QERR_SOCKET_BIND_FAILED); > >> - } > >> + error_set(errp, QERR_SOCKET_BIND_FAILED); > >> } > >> } > >> closesocket(slisten); > > > > This isn't obvious. It looks like the intent of the if > > (!e->ai_next) is > > to suppress the error so that the next iteration of the *outer* > > loop can > > succeed. > > > > Why is it okay to set QERR_SOCKET_BIND_FAILED? We may have more > > addrinfos left to try in the outer loop. They may succeed so we > > don't > > want an error in that case. > > You are correct, and the patch is wrong. Yeah, will drop this patch from this thread. Thanks. > See also related > http://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg00772.html > >
diff --git a/qemu-sockets.c b/qemu-sockets.c index 668fa93..c636882 100644 --- a/qemu-sockets.c +++ b/qemu-sockets.c @@ -181,9 +181,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp) fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__, inet_strfamily(e->ai_family), uaddr, inet_getport(e), strerror(errno)); - if (!e->ai_next) { - error_set(errp, QERR_SOCKET_BIND_FAILED); - } + error_set(errp, QERR_SOCKET_BIND_FAILED); } } closesocket(slisten);
It's aleady in the end of loop, error should be set. Signed-off-by: Amos Kong <akong@redhat.com> --- qemu-sockets.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)