Re: [Qemu-devel] Fwd: [RFC 2/4] qemu-socket: Allow custom socket options in socket_connect

[Daniel P . Berrangé]

On Wed, Jan 31, 2018 at 11:20:16PM +0800, Zihan Yang wrote:
> Hi, Daniel
> 
> >You've added all this extra functionality to pass arbitrary
> >options, but then not used it in any of the later patches.
> >We've been trying to remove complexity from this code, so
> >I'm not in favour of adding new functionality that is not
> >even used.
> 
> You are right, unused functionality should not be added. I was thinking
> about future usage, but that seems really unnecessary now.
> 
> >I'm not seeing the point of adding the support for the O_NONBLOCK
> >in the listener socket either - that can easily be turned on after
> >you have the listener socket created.
> 
> I don't quite understand how I can turn it on in socket_listen, because
> socket_listen will create a listener socket inside it, then bind and listen.
> Are there other ways than passing some kind of 'config parameter'?

You don't need to turn it on in socket_listen(). You can just do

   fd = socket_listen()
   qemu_set_nonblock(fd)
   ...do stuff...

> >The O_NONBLOCK functionality makes more sense in this context
> >but the implementation is really broken.
> 
> Well.. sorry for the broken implementation, I guess I need more practice.
> 
> >These functions do hostname lookups, so can never do non-blocking
> >mode correctly as the hostname lookup itself does blocking I/O
> >to the nameserver(s).  Ignoring that, the way you handle the
> >connect() is wrong too. You're iterating over many addresses
> >returned by getaddrinfo() and doing a non-blocking connect
> >on each of them. These will essentially all fail with EAGAIN
> >and you'll skip onto the next address which is wrong.
> 
> Why would the hostname lookup affect the listener socket
> afterwards? The socket is created after the lookup procedure is done.
> Therefore, the config should only affect the listener socket, not the
> hostname lookup process. Would you explain in more detail? I'm not
> an expert in socket programming, so I'm confused.
> 
> Also, connect() indeed could return EAGAIN, however, the continue
> expression is inside the do-while loop of inet_connect_addr(),
> rather than the for loop inside inet_connect_saddr(), which is the
> caller of inet_connect_addr(). So it would just try to connect again
> instead of skipping to next address.

The point of using  O_NONBLOCK is so that the caller does not get
delayed for a long time while network traffic runs.  There are two
places network traffic is used in socket_connect(). First it is
used to resolve the hostname to an IP address via DNS servers, and
second it is used in the TCP connection handshake.

The O_NONBLOCK code only affects the connection handshake, so the
caller can still be delayed an abitrary amount of time by the DNS
lookup.

IOW, if the caller must *not* be delayed, then simply using O_NONBLOCK
is not sufficient to avoid the delay. If the caller can tolerate
delays, then using O_NONBLOCK is pointless.


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