On Mon, Jun 26, 2017 at 01:56:28PM +0200, Knut Omang wrote:
> 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.
>
> In principle I agree with you, but I think we do want to keep the details
> of what the failure cause was by also propagating information about the
> system
> call that failed.
>
> I considered this an acceptable trade-off in the name of performance as well
> as
> readability at the next level. This is a fairly unlikely case that one really
> does not
> have to worry too much about at the next level. Setting an error that does
> not get used
> for that special, unlikely case is not that bad. Doing it for all failures
> would be
> a lot more unnecessary work.
>
> >
> > > + 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.
>
> I agree, a simple way to solve it would be to only set errp if no
> error has already been set.
I'd like to see each of the methods set the error unconditionally.
In the inet_socket_listen() method, we can use an then intermediate
Error variable on each iteration of the loop, and propagate to
the global variable at the end or discard it.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
