[Qemu-devel] [PATCH 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors

[Qemu-devel] [PATCH 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors

[Stefan Hajnoczi]

There are several places where QEMU accidentally relies on the O_NONBLOCK state
of passed file descriptors.  Exposing O_NONBLOCK state makes it part of the QMP
API whenever getfd or fdset_add_fd are used!

Whether or not QEMU will use O_NONBLOCK is an implementation detail and should
be hidden from QMP clients.

This patch series addresses this in 3 steps:

1. Fix callers of monitor_handle_fd_param(), monitor_fdset_get_fd(), and
   monitor_get_fd() that depend on O_NONBLOCK being set.  Luckily there are
   only two instances and they are fixed in Patches 1 & 2.

2. Rename socket_set_nonblock() to qemu_set_nonblock() just like
   qemu_set_cloexec().  This makes code cleaner when working with arbitrary
   file descriptors that may not be sockets.  See Patch 3.

3. Clear O_NONBLOCK when a chardev receives file descriptors.  From now on QEMU
   can assume that passed file descriptors are in blocking mode.  Simply use
   qemu_set_nonblock(fd) if you want to enable O_NONBLOCK.  See Patch 4.

This fixes live migration with recent libvirt.  Libvirt checks if QEMU supports
file descriptor passing and, if yes, hands QEMU a socket with O_NONBLOCK set.
The migrate fd:<foo> code assumes the socket is in blocking mode.  The result
is a corrupted migration stream.  For more info on this bug, see:

https://bugzilla.redhat.com/show_bug.cgi?id=923124

Note that Michal Privoznik <mprivozn@redhat.com> also sent a libvirt patch so
that old QEMUs work with new libvirts:

https://www.redhat.com/archives/libvir-list/2013-March/msg01486.html

My patch series fixes the QMP API and allows old libvirts to work again with
new QEMUs.

Stefan Hajnoczi (4):
  net: ensure "socket" backend uses non-blocking fds
  qemu-socket: set passed fd non-blocking in socket_connect()
  oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock()
  chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors

 block/nbd.c            |  2 +-
 block/sheepdog.c       |  2 +-
 include/qemu/sockets.h |  4 ++--
 migration.c            |  2 +-
 nbd.c                  |  8 ++++----
 net/socket.c           |  8 +++++---
 qemu-char.c            | 11 +++++++----
 savevm.c               |  2 +-
 slirp/misc.c           |  2 +-
 slirp/tcp_subr.c       |  4 ++--
 ui/vnc.c               |  2 +-
 util/oslib-posix.c     |  4 ++--
 util/oslib-win32.c     |  4 ++--
 util/qemu-sockets.c    |  5 +++--
 14 files changed, 33 insertions(+), 27 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/4] net: ensure "socket" backend uses non-blocking fds

[Stefan Hajnoczi]

There are several code paths in net_init_socket() depending on how the
socket is created: file descriptor passing, UDP multicast, TCP, or UDP.
Some of these support both listen and connect.

Not all code paths set the socket to non-blocking.  This patch addresses
the file descriptor passing and UDP cases which were missing
socket_set_nonblock(fd) calls.

I considered moving socket_set_nonblock(fd) to a central location but it
turns out the code paths are different enough to require non-blocking at
different places.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 net/socket.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/socket.c b/net/socket.c
index 6c3752b..4a88142 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -674,6 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
         closesocket(fd);
         return -1;
     }
+    socket_set_nonblock(fd);
 
     s = net_socket_fd_init(peer, model, name, fd, 0);
     if (!s) {
@@ -712,6 +713,7 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
         int fd;
 
         fd = monitor_handle_fd_param(cur_mon, sock->fd);
+        socket_set_nonblock(fd);
         if (fd == -1 || !net_socket_fd_init(peer, "socket", name, fd, 1)) {
             return -1;
         }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/4] net: ensure "socket" backend uses non-blocking fds

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> There are several code paths in net_init_socket() depending on how the
> socket is created: file descriptor passing, UDP multicast, TCP, or UDP.
> Some of these support both listen and connect.
>
> Not all code paths set the socket to non-blocking.  This patch addresses
> the file descriptor passing and UDP cases which were missing
> socket_set_nonblock(fd) calls.
>
> I considered moving socket_set_nonblock(fd) to a central location but it
> turns out the code paths are different enough to require non-blocking at
> different places.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  net/socket.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/socket.c b/net/socket.c
> index 6c3752b..4a88142 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -674,6 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
>          closesocket(fd);
>          return -1;
>      }
> +    socket_set_nonblock(fd);
>  
>      s = net_socket_fd_init(peer, model, name, fd, 0);
>      if (!s) {
> @@ -712,6 +713,7 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
>          int fd;
>  
>          fd = monitor_handle_fd_param(cur_mon, sock->fd);
> +        socket_set_nonblock(fd);
>          if (fd == -1 || !net_socket_fd_init(peer, "socket", name, fd, 1)) {

Shouldn't be better to refactor the code to _not_ call
socket_set_nonblock() in the case that fd == -1?  It will just clobber
errno?

>              return -1;
>          }

Re: [Qemu-devel] [PATCH 1/4] net: ensure "socket" backend uses non-blocking fds

[Stefan Hajnoczi]

On Tue, Mar 26, 2013 at 05:25:50PM +0100, Juan Quintela wrote:
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > There are several code paths in net_init_socket() depending on how the
> > socket is created: file descriptor passing, UDP multicast, TCP, or UDP.
> > Some of these support both listen and connect.
> >
> > Not all code paths set the socket to non-blocking.  This patch addresses
> > the file descriptor passing and UDP cases which were missing
> > socket_set_nonblock(fd) calls.
> >
> > I considered moving socket_set_nonblock(fd) to a central location but it
> > turns out the code paths are different enough to require non-blocking at
> > different places.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  net/socket.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/socket.c b/net/socket.c
> > index 6c3752b..4a88142 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -674,6 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
> >          closesocket(fd);
> >          return -1;
> >      }
> > +    socket_set_nonblock(fd);
> >  
> >      s = net_socket_fd_init(peer, model, name, fd, 0);
> >      if (!s) {
> > @@ -712,6 +713,7 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
> >          int fd;
> >  
> >          fd = monitor_handle_fd_param(cur_mon, sock->fd);
> > +        socket_set_nonblock(fd);
> >          if (fd == -1 || !net_socket_fd_init(peer, "socket", name, fd, 1)) {
> 
> Shouldn't be better to refactor the code to _not_ call
> socket_set_nonblock() in the case that fd == -1?  It will just clobber
> errno?

I'll send a fixed version.

Stefan

Re: [Qemu-devel] [PATCH 1/4] net: ensure "socket" backend uses non-blocking fds

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On 03/26/2013 10:07 AM, Stefan Hajnoczi wrote:
> There are several code paths in net_init_socket() depending on how the
> socket is created: file descriptor passing, UDP multicast, TCP, or UDP.
> Some of these support both listen and connect.
> 
> Not all code paths set the socket to non-blocking.  This patch addresses
> the file descriptor passing and UDP cases which were missing
> socket_set_nonblock(fd) calls.
> 
> I considered moving socket_set_nonblock(fd) to a central location but it
> turns out the code paths are different enough to require non-blocking at
> different places.

Is it worth rearranging patch 3 first, so that you don't have to churn
on these newly-added lines?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/4] net: ensure "socket" backend uses non-blocking fds

[Stefan Hajnoczi]

On Tue, Mar 26, 2013 at 10:33:45AM -0600, Eric Blake wrote:
> On 03/26/2013 10:07 AM, Stefan Hajnoczi wrote:
> > There are several code paths in net_init_socket() depending on how the
> > socket is created: file descriptor passing, UDP multicast, TCP, or UDP.
> > Some of these support both listen and connect.
> > 
> > Not all code paths set the socket to non-blocking.  This patch addresses
> > the file descriptor passing and UDP cases which were missing
> > socket_set_nonblock(fd) calls.
> > 
> > I considered moving socket_set_nonblock(fd) to a central location but it
> > turns out the code paths are different enough to require non-blocking at
> > different places.
> 
> Is it worth rearranging patch 3 first, so that you don't have to churn
> on these newly-added lines?

Will do that in v2.

Stefan

[Qemu-devel] [PATCH 2/4] qemu-socket: set passed fd non-blocking in socket_connect()

[Stefan Hajnoczi]

socket_connect() sets non-blocking on TCP or UNIX domain sockets if a
callback function is passed.  Do the same for file descriptor passing,
otherwise we could unexpectedly be using a blocking file descriptor.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/qemu-sockets.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b6b78f5..298f6f2 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -910,6 +910,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
     case SOCKET_ADDRESS_KIND_FD:
         fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
         if (callback) {
+            socket_set_nonblock(fd);
             callback(fd, opaque);
         }
         break;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/4] qemu-socket: set passed fd non-blocking in socket_connect()

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> socket_connect() sets non-blocking on TCP or UNIX domain sockets if a
> callback function is passed.  Do the same for file descriptor passing,
> otherwise we could unexpectedly be using a blocking file descriptor.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/qemu-sockets.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index b6b78f5..298f6f2 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -910,6 +910,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
>      case SOCKET_ADDRESS_KIND_FD:
>          fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
>          if (callback) {
> +            socket_set_nonblock(fd);
>              callback(fd, opaque);
>          }
>          break;

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] qemu-socket: set passed fd non-blocking in socket_connect()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On 03/26/2013 10:07 AM, Stefan Hajnoczi wrote:
> socket_connect() sets non-blocking on TCP or UNIX domain sockets if a
> callback function is passed.  Do the same for file descriptor passing,
> otherwise we could unexpectedly be using a blocking file descriptor.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/qemu-sockets.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index b6b78f5..298f6f2 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -910,6 +910,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
>      case SOCKET_ADDRESS_KIND_FD:
>          fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
>          if (callback) {
> +            socket_set_nonblock(fd);
>              callback(fd, opaque);
>          }
>          break;
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 3/4] oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock()

[Stefan Hajnoczi]

The fcntl(fd, F_SETFL, O_NONBLOCK) flag is not specific to sockets.
Rename to qemu_set_nonblock() just like qemu_set_cloexec().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/nbd.c            |  2 +-
 block/sheepdog.c       |  2 +-
 include/qemu/sockets.h |  4 ++--
 migration.c            |  2 +-
 nbd.c                  |  8 ++++----
 net/socket.c           | 10 +++++-----
 qemu-char.c            |  8 ++++----
 savevm.c               |  2 +-
 slirp/misc.c           |  2 +-
 slirp/tcp_subr.c       |  4 ++--
 ui/vnc.c               |  2 +-
 util/oslib-posix.c     |  4 ++--
 util/oslib-win32.c     |  4 ++--
 util/qemu-sockets.c    |  6 +++---
 14 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 3d711b2..eff683c 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -415,7 +415,7 @@ static int nbd_establish_connection(BlockDriverState *bs)
 
     /* Now that we're connected, set the socket to be non-blocking and
      * kick the reply mechanism.  */
-    socket_set_nonblock(sock);
+    qemu_set_nonblock(sock);
     qemu_aio_set_fd_handler(sock, nbd_reply_ready, NULL,
                             nbd_have_request, s);
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index bb67c4c..987018e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -471,7 +471,7 @@ static int connect_to_sdog(BDRVSheepdogState *s)
         qerror_report_err(err);
         error_free(err);
     } else {
-        socket_set_nonblock(fd);
+        qemu_set_nonblock(fd);
     }
 
     return fd;
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index d225f6d..c5174d7 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -37,8 +37,8 @@ int qemu_socket(int domain, int type, int protocol);
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
 int socket_set_cork(int fd, int v);
 int socket_set_nodelay(int fd);
-void socket_set_block(int fd);
-void socket_set_nonblock(int fd);
+void qemu_set_block(int fd);
+void qemu_set_nonblock(int fd);
 int send_all(int fd, const void *buf, int len1);
 int recv_all(int fd, void *buf, int len1, bool single_read);
 
diff --git a/migration.c b/migration.c
index 185d112..0aff06d 100644
--- a/migration.c
+++ b/migration.c
@@ -121,7 +121,7 @@ void process_incoming_migration(QEMUFile *f)
     int fd = qemu_get_fd(f);
 
     assert(fd != -1);
-    socket_set_nonblock(fd);
+    qemu_set_nonblock(fd);
     qemu_coroutine_enter(co, f);
 }
 
diff --git a/nbd.c b/nbd.c
index d1a67ee..85187ff 100644
--- a/nbd.c
+++ b/nbd.c
@@ -386,7 +386,7 @@ static int nbd_send_negotiate(NBDClient *client)
         [28 .. 151]   reserved     (0)
      */
 
-    socket_set_block(csock);
+    qemu_set_block(csock);
     rc = -EINVAL;
 
     TRACE("Beginning negotiation.");
@@ -429,7 +429,7 @@ static int nbd_send_negotiate(NBDClient *client)
     TRACE("Negotiation succeeded.");
     rc = 0;
 fail:
-    socket_set_nonblock(csock);
+    qemu_set_nonblock(csock);
     return rc;
 }
 
@@ -443,7 +443,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
 
     TRACE("Receiving negotiation.");
 
-    socket_set_block(csock);
+    qemu_set_block(csock);
     rc = -EINVAL;
 
     if (read_sync(csock, buf, 8) != 8) {
@@ -558,7 +558,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
     rc = 0;
 
 fail:
-    socket_set_nonblock(csock);
+    qemu_set_nonblock(csock);
     return rc;
 }
 
diff --git a/net/socket.c b/net/socket.c
index 4a88142..13ac53e 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -308,7 +308,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
         }
     }
 
-    socket_set_nonblock(fd);
+    qemu_set_nonblock(fd);
     return fd;
 fail:
     if (fd >= 0)
@@ -519,7 +519,7 @@ static int net_socket_listen_init(NetClientState *peer,
         perror("socket");
         return -1;
     }
-    socket_set_nonblock(fd);
+    qemu_set_nonblock(fd);
 
     /* allow fast reuse */
     val = 1;
@@ -565,7 +565,7 @@ static int net_socket_connect_init(NetClientState *peer,
         perror("socket");
         return -1;
     }
-    socket_set_nonblock(fd);
+    qemu_set_nonblock(fd);
 
     connected = 0;
     for(;;) {
@@ -674,7 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
         closesocket(fd);
         return -1;
     }
-    socket_set_nonblock(fd);
+    qemu_set_nonblock(fd);
 
     s = net_socket_fd_init(peer, model, name, fd, 0);
     if (!s) {
@@ -713,7 +713,7 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
         int fd;
 
         fd = monitor_handle_fd_param(cur_mon, sock->fd);
-        socket_set_nonblock(fd);
+        qemu_set_nonblock(fd);
         if (fd == -1 || !net_socket_fd_init(peer, "socket", name, fd, 1)) {
             return -1;
         }
diff --git a/qemu-char.c b/qemu-char.c
index 4e011df..8fb8340 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2537,7 +2537,7 @@ static int tcp_chr_add_client(CharDriverState *chr, int fd)
     if (s->fd != -1)
 	return -1;
 
-    socket_set_nonblock(fd);
+    qemu_set_nonblock(fd);
     if (s->do_nodelay)
         socket_set_nodelay(fd);
     s->fd = fd;
@@ -2689,7 +2689,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
         printf("QEMU waiting for connection on: %s\n",
                chr->filename);
         tcp_chr_accept(s->listen_chan, G_IO_IN, chr);
-        socket_set_nonblock(s->listen_fd);
+        qemu_set_nonblock(s->listen_fd);
     }
     return chr;
 }
@@ -2731,7 +2731,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     }
 
     if (!is_waitconnect)
-        socket_set_nonblock(fd);
+        qemu_set_nonblock(fd);
 
     chr = qemu_chr_open_socket_fd(fd, do_nodelay, is_listen, is_telnet,
                                   is_waitconnect, &local_err);
@@ -3623,7 +3623,7 @@ static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
     if (error_is_set(errp)) {
         return NULL;
     }
-    socket_set_nonblock(fd);
+    qemu_set_nonblock(fd);
     return qemu_chr_open_tty_fd(fd);
 #else
     error_setg(errp, "character device backend type 'serial' not supported");
diff --git a/savevm.c b/savevm.c
index 35c8d1e..706cf4b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -403,7 +403,7 @@ QEMUFile *qemu_fopen_socket(int fd, const char *mode)
 
     s->fd = fd;
     if (mode[0] == 'w') {
-        socket_set_block(s->fd);
+        qemu_set_block(s->fd);
         s->file = qemu_fopen_ops(s, &socket_write_ops);
     } else {
         s->file = qemu_fopen_ops(s, &socket_read_ops);
diff --git a/slirp/misc.c b/slirp/misc.c
index 6b9c2c4..8ecced5 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -215,7 +215,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                 qemu_setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
                 opt = 1;
                 qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
-		socket_set_nonblock(so->s);
+		qemu_set_nonblock(so->s);
 
 		/* Append the telnet options now */
                 if (so->so_m != NULL && do_pty == 1)  {
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 84a6bb5..e98ce1a 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -336,7 +336,7 @@ int tcp_fconnect(struct socket *so)
     int opt, s=so->s;
     struct sockaddr_in addr;
 
-    socket_set_nonblock(s);
+    qemu_set_nonblock(s);
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
     opt = 1;
@@ -425,7 +425,7 @@ void tcp_connect(struct socket *inso)
         tcp_close(sototcpcb(so)); /* This will sofree() as well */
         return;
     }
-    socket_set_nonblock(s);
+    qemu_set_nonblock(s);
     opt = 1;
     qemu_setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(int));
     opt = 1;
diff --git a/ui/vnc.c b/ui/vnc.c
index bbe1e0f..5ddb696 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2732,7 +2732,7 @@ static void vnc_connect(VncDisplay *vd, int csock, int skipauth, bool websocket)
 
     VNC_DEBUG("New client on socket %d\n", csock);
     vd->dcl.idle = 0;
-    socket_set_nonblock(vs->csock);
+    qemu_set_nonblock(vs->csock);
 #ifdef CONFIG_VNC_WS
     if (websocket) {
         vs->websocket = 1;
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 433dd68..4e4b819 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -134,14 +134,14 @@ void qemu_vfree(void *ptr)
     free(ptr);
 }
 
-void socket_set_block(int fd)
+void qemu_set_block(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
     fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
 }
 
-void socket_set_nonblock(int fd)
+void qemu_set_nonblock(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFL);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 640194c..dcfa0c2 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -100,14 +100,14 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
     return p;
 }
 
-void socket_set_block(int fd)
+void qemu_set_block(int fd)
 {
     unsigned long opt = 0;
     WSAEventSelect(fd, NULL, 0);
     ioctlsocket(fd, FIONBIO, &opt);
 }
 
-void socket_set_nonblock(int fd)
+void qemu_set_nonblock(int fd)
 {
     unsigned long opt = 1;
     ioctlsocket(fd, FIONBIO, &opt);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 298f6f2..94581aa 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -277,7 +277,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
     }
     qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
     if (connect_state != NULL) {
-        socket_set_nonblock(sock);
+        qemu_set_nonblock(sock);
     }
     /* connect to peer */
     do {
@@ -737,7 +737,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
         connect_state = g_malloc0(sizeof(*connect_state));
         connect_state->callback = callback;
         connect_state->opaque = opaque;
-        socket_set_nonblock(sock);
+        qemu_set_nonblock(sock);
     }
 
     memset(&un, 0, sizeof(un));
@@ -910,7 +910,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
     case SOCKET_ADDRESS_KIND_FD:
         fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
         if (callback) {
-            socket_set_nonblock(fd);
+            qemu_set_nonblock(fd);
             callback(fd, opaque);
         }
         break;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 3/4] oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock()

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The fcntl(fd, F_SETFL, O_NONBLOCK) flag is not specific to sockets.
> Rename to qemu_set_nonblock() just like qemu_set_cloexec().
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/nbd.c            |  2 +-
>  block/sheepdog.c       |  2 +-
>  include/qemu/sockets.h |  4 ++--
>  migration.c            |  2 +-
>  nbd.c                  |  8 ++++----
>  net/socket.c           | 10 +++++-----
>  qemu-char.c            |  8 ++++----
>  savevm.c               |  2 +-
>  slirp/misc.c           |  2 +-
>  slirp/tcp_subr.c       |  4 ++--
>  ui/vnc.c               |  2 +-
>  util/oslib-posix.c     |  4 ++--
>  util/oslib-win32.c     |  4 ++--
>  util/qemu-sockets.c    |  6 +++---
>  14 files changed, 30 insertions(+), 30 deletions(-)
>

Reviewed-by: Juan Quintela <quintela@redhat.com> 

Re: [Qemu-devel] [PATCH 3/4] oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On 03/26/2013 10:07 AM, Stefan Hajnoczi wrote:
> The fcntl(fd, F_SETFL, O_NONBLOCK) flag is not specific to sockets.
> Rename to qemu_set_nonblock() just like qemu_set_cloexec().
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/util/oslib-posix.c
> @@ -134,14 +134,14 @@ void qemu_vfree(void *ptr)
>      free(ptr);
>  }
>  
> -void socket_set_block(int fd)
> +void qemu_set_block(int fd)
>  {
>      int f;
>      f = fcntl(fd, F_GETFL);
>      fcntl(fd, F_SETFL, f & ~O_NONBLOCK);

Odd that we aren't checking for errors here, but that's an independent
consideration not for this patch.

> +++ b/util/oslib-win32.c
> @@ -100,14 +100,14 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
>      return p;
>  }
>  
> -void socket_set_block(int fd)
> +void qemu_set_block(int fd)
>  {
>      unsigned long opt = 0;
>      WSAEventSelect(fd, NULL, 0);
>      ioctlsocket(fd, FIONBIO, &opt);

ioctlsocket() doesn't work on non-sockets, does it?  On the other hand,
do we ever call qemu_set_block() on a non-socket in the mingw build,
since we lack SCM_RIGHTS on that platform?  Also, this is another case
of not checking for errors - I guess inability to set blocking on a
non-socket is not fatal, so not checking for errors kind of makes sense.
 Again, doesn't impact this patch from being a mechanical rename.

See my comment in 1/4 about possibly rebasing this to be first (in which
case it is slightly smaller).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 4/4] chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors

[Stefan Hajnoczi]

When we receive a file descriptor over a UNIX domain socket the
O_NONBLOCK flag is preserved.  Clear the O_NONBLOCK flag and rely on
QEMU file descriptor users like migration, SPICE, VNC, block layer, and
others to set non-blocking only when necessary.

This change ensures we don't accidentally expose O_NONBLOCK in the QMP
API.  QMP clients should not need to get the non-blocking state
"correct".

A recent real-world example was when libvirt passed a non-blocking TCP
socket for migration where we expected a blocking socket.  The source
QEMU produced a corrupted migration stream since its code did not cope
with non-blocking sockets.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-char.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 8fb8340..e50b1b3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2407,6 +2407,9 @@ static void unix_process_msgfd(CharDriverState *chr, struct msghdr *msg)
         if (fd < 0)
             continue;
 
+        /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
+        qemu_set_block(fd);
+
 #ifndef MSG_CMSG_CLOEXEC
         qemu_set_cloexec(fd);
 #endif
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 4/4] chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> When we receive a file descriptor over a UNIX domain socket the
> O_NONBLOCK flag is preserved.  Clear the O_NONBLOCK flag and rely on
> QEMU file descriptor users like migration, SPICE, VNC, block layer, and
> others to set non-blocking only when necessary.
>
> This change ensures we don't accidentally expose O_NONBLOCK in the QMP
> API.  QMP clients should not need to get the non-blocking state
> "correct".
>
> A recent real-world example was when libvirt passed a non-blocking TCP
> socket for migration where we expected a blocking socket.  The source
> QEMU produced a corrupted migration stream since its code did not cope
> with non-blocking sockets.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Acked-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On 03/26/2013 10:07 AM, Stefan Hajnoczi wrote:
> When we receive a file descriptor over a UNIX domain socket the
> O_NONBLOCK flag is preserved.  Clear the O_NONBLOCK flag and rely on
> QEMU file descriptor users like migration, SPICE, VNC, block layer, and
> others to set non-blocking only when necessary.
> 
> This change ensures we don't accidentally expose O_NONBLOCK in the QMP
> API.  QMP clients should not need to get the non-blocking state
> "correct".
> 
> A recent real-world example was when libvirt passed a non-blocking TCP
> socket for migration where we expected a blocking socket.  The source
> QEMU produced a corrupted migration stream since its code did not cope
> with non-blocking sockets.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-char.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

Covers both 'getfd' and 'add-fd' paths.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors

[liu ping fan]

On Wed, Mar 27, 2013 at 12:07 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> There are several places where QEMU accidentally relies on the O_NONBLOCK state
> of passed file descriptors.  Exposing O_NONBLOCK state makes it part of the QMP

If in future, we push more backend on their dedicated thread, will the
related fd be block?
> API whenever getfd or fdset_add_fd are used!
>
> Whether or not QEMU will use O_NONBLOCK is an implementation detail and should
> be hidden from QMP clients.
>
> This patch series addresses this in 3 steps:
>
> 1. Fix callers of monitor_handle_fd_param(), monitor_fdset_get_fd(), and
>    monitor_get_fd() that depend on O_NONBLOCK being set.  Luckily there are
>    only two instances and they are fixed in Patches 1 & 2.
>
> 2. Rename socket_set_nonblock() to qemu_set_nonblock() just like
>    qemu_set_cloexec().  This makes code cleaner when working with arbitrary
>    file descriptors that may not be sockets.  See Patch 3.
>
> 3. Clear O_NONBLOCK when a chardev receives file descriptors.  From now on QEMU
>    can assume that passed file descriptors are in blocking mode.  Simply use
>    qemu_set_nonblock(fd) if you want to enable O_NONBLOCK.  See Patch 4.
>
> This fixes live migration with recent libvirt.  Libvirt checks if QEMU supports
> file descriptor passing and, if yes, hands QEMU a socket with O_NONBLOCK set.
> The migrate fd:<foo> code assumes the socket is in blocking mode.  The result
> is a corrupted migration stream.  For more info on this bug, see:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=923124
>
> Note that Michal Privoznik <mprivozn@redhat.com> also sent a libvirt patch so
> that old QEMUs work with new libvirts:
>
> https://www.redhat.com/archives/libvir-list/2013-March/msg01486.html
>
> My patch series fixes the QMP API and allows old libvirts to work again with
> new QEMUs.
>
> Stefan Hajnoczi (4):
>   net: ensure "socket" backend uses non-blocking fds
>   qemu-socket: set passed fd non-blocking in socket_connect()
>   oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock()
>   chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors
>
>  block/nbd.c            |  2 +-
>  block/sheepdog.c       |  2 +-
>  include/qemu/sockets.h |  4 ++--
>  migration.c            |  2 +-
>  nbd.c                  |  8 ++++----
>  net/socket.c           |  8 +++++---
>  qemu-char.c            | 11 +++++++----
>  savevm.c               |  2 +-
>  slirp/misc.c           |  2 +-
>  slirp/tcp_subr.c       |  4 ++--
>  ui/vnc.c               |  2 +-
>  util/oslib-posix.c     |  4 ++--
>  util/oslib-win32.c     |  4 ++--
>  util/qemu-sockets.c    |  5 +++--
>  14 files changed, 33 insertions(+), 27 deletions(-)
>
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors

[Stefan Hajnoczi]

On Wed, Mar 27, 2013 at 04:37:52PM +0800, liu ping fan wrote:
> On Wed, Mar 27, 2013 at 12:07 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > There are several places where QEMU accidentally relies on the O_NONBLOCK state
> > of passed file descriptors.  Exposing O_NONBLOCK state makes it part of the QMP
> 
> If in future, we push more backend on their dedicated thread, will the
> related fd be block?

This series is not related to threading in QEMU.  The convention it
establishes is that passed fds are blocking.  If QEMU wants to use
nonblocking it must call qemu_set_block(fd).  This works whether it is
done from a traditional QEMU thread or a data plane thread.

Stefan

Re: [Qemu-devel] [PATCH 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Mar 27, 2013 at 04:37:52PM +0800, liu ping fan wrote:
>> On Wed, Mar 27, 2013 at 12:07 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > There are several places where QEMU accidentally relies on the
>> > O_NONBLOCK state
>> > of passed file descriptors.  Exposing O_NONBLOCK state makes it
>> > part of the QMP
>> 
>> If in future, we push more backend on their dedicated thread, will the
>> related fd be block?
>
> This series is not related to threading in QEMU.  The convention it
> establishes is that passed fds are blocking.  If QEMU wants to use
                                     ^^^^^^^^
You mean here non-blocking

> nonblocking it must call qemu_set_block(fd).  This works whether it is
                           ^^^^^^^^^^^^^^^^^^
or here qemu_set_nonblock(fd)

no?

I guess the second one O:-)

Later, Juan.

> done from a traditional QEMU thread or a data plane thread.
>
> Stefan

Re: [Qemu-devel] [PATCH 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors

[Stefan Hajnoczi]

On Wed, Mar 27, 2013 at 02:06:36PM +0100, Juan Quintela wrote:
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Wed, Mar 27, 2013 at 04:37:52PM +0800, liu ping fan wrote:
> >> On Wed, Mar 27, 2013 at 12:07 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >> > There are several places where QEMU accidentally relies on the
> >> > O_NONBLOCK state
> >> > of passed file descriptors.  Exposing O_NONBLOCK state makes it
> >> > part of the QMP
> >> 
> >> If in future, we push more backend on their dedicated thread, will the
> >> related fd be block?
> >
> > This series is not related to threading in QEMU.  The convention it
> > establishes is that passed fds are blocking.  If QEMU wants to use
>                                      ^^^^^^^^
> You mean here non-blocking
> 
> > nonblocking it must call qemu_set_block(fd).  This works whether it is
>                            ^^^^^^^^^^^^^^^^^^
> or here qemu_set_nonblock(fd)
> 
> no?
> 
> I guess the second one O:-)

Blocking by default, call qemu_set_nonblock(fd) if you need
non-blocking.

:)

Sorry for the mistake.

Stefan