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

[Daniel P . Berrangé]

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

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

[Zihan Yang]

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.

Note the caller who wants a non-blocking socket should always
check the errno after socket_connect returns to see if the
connection is really established or still inprogress.

Signed-off-by: Zihan Yang 
---
 block/sheepdog.c   |  2 +-
 block/ssh.c|  2 +-
 include/qemu/sockets.h |  5 +--
 io/channel-socket.c|  2 +-
 util/qemu-sockets.c| 82 --
 5 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index f684477..f96e091 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -591,7 +591,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
**errp)
 {
 int fd;
 
-fd = socket_connect(s->addr, errp);
+fd = socket_connect(s->addr, errp, NULL);
 
 if (s->addr->type == SOCKET_ADDRESS_TYPE_INET && fd >= 0) {
 int ret = socket_set_nodelay(fd);
diff --git a/block/ssh.c b/block/ssh.c
index b049a16..c9d00c8 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -678,7 +678,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 }
 
 /* Open the socket and connect. */
-s->sock = inet_connect_saddr(s->inet, errp);
+s->sock = inet_connect_saddr(s->inet, errp, NULL);
 if (s->sock < 0) {
 ret = -EIO;
 goto err;
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index ab06943..13d6716 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -46,7 +46,8 @@ int inet_ai_family_from_address(InetSocketAddress *addr,
 Error **errp);
 int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
 int inet_connect(const char *str, Error **errp);
-int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
+int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+   QemuSocketConfig *sconf);
 
 NetworkAddressFamily inet_netfamily(int family);
 
@@ -54,7 +55,7 @@ int unix_listen(const char *path, Error **errp);
 int unix_connect(const char *path, Error **errp);
 
 SocketAddress *socket_parse(const char *str, Error **errp);
-int socket_connect(SocketAddress *addr, Error **errp);
+int socket_connect(SocketAddress *addr, Error **errp, QemuSocketConfig *sconf);
 int socket_listen(SocketAddress *addr, Error **errp, QemuSocketConfig *sconf);
 void socket_listen_cleanup(int fd, Error **errp);
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 9942cf0..f010a8e 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -140,7 +140,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
 int fd;
 
 trace_qio_channel_socket_connect_sync(ioc, addr);
-fd = socket_connect(addr, errp);
+fd = socket_connect(addr, errp, NULL);
 if (fd < 0) {
 trace_qio_channel_socket_connect_fail(ioc);
 return -1;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b171f23..3a10206 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -372,9 +372,11 @@ listen_ok:
 ((rc) == -EINPROGRESS)
 #endif
 
-static int inet_connect_addr(struct addrinfo *addr, Error **errp);
+static int inet_connect_addr(struct addrinfo *addr, Error **errp,
+ QemuSocketConfig *sconf);
 
-static int inet_connect_addr(struct addrinfo *addr, Error **errp)
+static int inet_connect_addr(struct addrinfo *addr, Error **errp,
+ QemuSocketConfig *sconf)
 {
 int sock, rc;
 
@@ -385,12 +387,26 @@ static int inet_connect_addr(struct addrinfo *addr, Error 
**errp)
 }
 socket_set_fast_reuse(sock);
 
+if(parse_socket_options(sock, errp, sconf) < 0) {
+error_setg_errno(errp, errno, "Failed to set socket options");
+return -1;
+}
+
 /* connect to peer */
 do {
 rc = 0;
 if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
 rc = -errno;
 }
+/* More judge for nonblocking socket, borrowed from 
net_socket_connect_init */
+if(sconf->nonblocking) {
+if(rc == -EWOULDBLOCK)
+continue;
+else if(rc == -EINPROGRESS ||
+rc == -EALREADY ||
+rc == -EINVAL)
+return sock; /* caller needs tp judge errno */
+}
 } while (rc == -EINTR);
 
 if (rc < 0) {
@@ -459,7 +475,8 @@ static struct addrinfo 
*inet_parse_connect_saddr(InetSocketAddress *saddr,
  *
  * Returns: -1 on error, file descriptor on success.
  */
-int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
+int