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