On Mon, 2017-06-26 at 11:28 +0100, Daniel P. Berrange wrote:
> On Fri, Jun 23, 2017 at 12:31:06PM +0200, Knut Omang wrote:
> >
> > First refactoring step to prepare for fixing the problem
> > exposed with the test-listen test in the previous commit
> >
> > Signed-off-by: Knut Omang <knut.om...@oracle.com>
> > ---
> > util/qemu-sockets.c | 24 +++++++++++++++++-------
> > 1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 852773d..699e36c 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress
> > *addr,
> > return PF_UNSPEC;
> > }
> >
> > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp)
> > +{
> > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e-
> > >ai_protocol);
> > + if (slisten < 0) {
> > + if (!e->ai_next) {
> > + error_setg_errno(errp, errno, "Failed to create socket");
> > + }
>
> I think that having this method sometimes report an error message, and
> sometimes not report an error message, depending on state of a variable
> used by the caller is rather unpleasant. I'd much rather see this
> error message reporting remain in the caller.
>
> >
> > + return -1;
> > + }
> > +
> > + socket_set_fast_reuse(slisten);
> > + return slisten;
> > +}
> > +
> > static int inet_listen_saddr(InetSocketAddress *saddr,
> > int port_offset,
> > bool update_addr,
> > @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> > return -1;
> > }
> >
> > - /* create socket + bind */
> > + /* create socket + bind/listen */
> > for (e = res; e != NULL; e = e->ai_next) {
> > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
> > uaddr,INET6_ADDRSTRLEN,uport,32,
> > NI_NUMERICHOST | NI_NUMERICSERV);
> > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e-
> > >ai_protocol);
> > +
> > + slisten = create_fast_reuse_socket(e, &err);
> > if (slisten < 0) {
> > - if (!e->ai_next) {
> > - error_setg_errno(errp, errno, "Failed to create socket");
> > - }
> > continue;
>
> It isn't shown in this diff context, but at the end of the outer
> loop we have
>
> error_setg_errno(errp, errno, "Failed to find available port");
>
> so IIUC, even this pre-existing code is wrong. If 'e->ai_next' is
> NULL, we report an error message here. Then, we continue to the
> next loop iteration, which causes use to terminate the loop
> entirely. At which point we'll report another error message
> over the top of the one we already have. So I think the error
> reporting does still need refactoring, but not in the way it
> is done here.
Yes, I did scratch my head about this but I tried to keep the original semantics
to avoid mixing unrelated changes.
With the split into separate refactoring commits we are beyond that anyway.
I'll have a second look at it..
Thanks,
Knut
>
> >
> > }
> >
> > - socket_set_fast_reuse(slisten);
> > -
> > port_min = inet_getport(e);
> > port_max = saddr->has_to ? saddr->to + port_offset : port_min;
> > for (p = port_min; p <= port_max; p++) {
>
> Regards,
> Daniel
