Patchwork [RESEND,1/3] socket: remove redundant check

login
register
mail settings
Submitter Amos Kong
Date Aug. 1, 2012, 8:59 a.m.
Message ID <1343811543-12137-2-git-send-email-akong@redhat.com>
Download mbox | patch
Permalink /patch/174401/
State New
Headers show

Comments

Amos Kong - Aug. 1, 2012, 8:59 a.m.
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(-)
Stefan Hajnoczi - Aug. 1, 2012, 10:50 a.m.
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
Peter Maydell - Aug. 1, 2012, 11:23 a.m.
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
Markus Armbruster - Aug. 1, 2012, 11:48 a.m.
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
Markus Armbruster - Aug. 1, 2012, 11:50 a.m.
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

[...]
Amos Kong - Aug. 3, 2012, 2:59 a.m.
----- 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
> 
>

Patch

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);