On Tue, Jan 30, 2018 at 03:13:42AM +0800, Zihan Yang wrote: > Currently, socket_connect doesn't allow custom socket options, > which is inconvenient when the caller wants a different kind of > socket from that the socket_connect provides. This patch allows > custom config in socket_connect by providing an extra parameter. > Existing functions can just pass a NULL pointer.
Again adding options functionality hwich is not actually used is bad. The O_NONBLOCK functionality makes more sense in this context but the implementation is really broken. 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. This code used to have support for O_NONBLOCK but it was removed because the DNS problem means that any code relying on it was already broken. The rest of the QEMU codebase has been converted to use QIOChannelSocket instead which can handle non-blocking DNS properly So overall I'm against this patch too. 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 :|