[Qemu-trivial] [RESEND PATCH 0/3] trivial fix of qemu-sockets.c

[Qemu-trivial] [RESEND PATCH 0/3] trivial fix of qemu-sockets.c

[Amos Kong]

Those patches fix trivial issues which were found
in the second review.

Amos Kong (3):
  socket: remove redundant check
  remove unused include of error.h
  socket: clean up redundant assignment

 qemu-sockets.c |    5 +----
 qemu_socket.h  |    1 -
 2 files changed, 1 insertions(+), 5 deletions(-)

[Qemu-trivial] [RESEND PATCH 1/3] socket: remove redundant check

[Amos Kong]

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

Re: [Qemu-trivial] [RESEND PATCH 1/3] socket: remove redundant check

[Stefan Hajnoczi]

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

Re: [Qemu-trivial] [Qemu-devel] [RESEND PATCH 1/3] socket: remove redundant check

[Peter Maydell]

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

Re: [Qemu-trivial] [Qemu-devel] [RESEND PATCH 1/3] socket: remove redundant check

[Markus Armbruster]

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

[...]

Re: [Qemu-trivial] [Qemu-devel] [RESEND PATCH 1/3] socket: remove redundant check

[Markus Armbruster]

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

Re: [Qemu-trivial] [Qemu-devel] [RESEND PATCH 1/3] socket: remove redundant check

[Amos Kong]

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

[Qemu-trivial] [RESEND PATCH 2/3] remove unused include of error.h

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qemu_socket.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/qemu_socket.h b/qemu_socket.h
index 4689ff3..1a2f517 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -27,7 +27,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #endif /* !_WIN32 */
 
 #include "qemu-option.h"
-#include "error.h"
 #include "qerror.h"
 
 /* misc helpers */
-- 
1.7.1

[Qemu-trivial] [RESEND PATCH 3/3] socket: clean up redundant assignment

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qemu-sockets.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index c636882..9cd4114 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -282,7 +282,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp)
                         inet_strfamily(e->ai_family),
                         e->ai_canonname, uaddr, uport, strerror(errno));
             closesocket(sock);
-            sock = -1;
             continue;
         }
         freeaddrinfo(res);
-- 
1.7.1