[Qemu-devel] [PULL 0/6] QMP queue

[Qemu-devel] [PULL 0/6] QMP queue

[Luiz Capitulino]

One more pull this week. This one contains two series fixing two important
bugs.

The changes (since 404e7a4f4af753bd2aef649adf79e7434fb6dc31) are available
in the following repository:

    git://repo.or.cz/qemu/qmp-unstable.git queue/qmp

Luiz Capitulino (2):
  qstring: add qobject_get_length()
  Monitor: Make output buffer dynamic

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

 block/nbd.c                |  2 +-
 block/sheepdog.c           |  2 +-
 include/qapi/qmp/qstring.h |  1 +
 include/qemu/sockets.h     |  4 ++--
 migration.c                |  2 +-
 monitor.c                  | 42 +++++++++++++++++++++++++-----------------
 nbd.c                      |  8 ++++----
 net/socket.c               | 13 +++++++++----
 qemu-char.c                | 11 +++++++----
 qobject/qstring.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        |  5 +++--
 17 files changed, 71 insertions(+), 45 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PULL 1/6] oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock()

[Luiz Capitulino]

From: Stefan Hajnoczi <stefanha@redhat.com>

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>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block/nbd.c            | 2 +-
 block/sheepdog.c       | 2 +-
 include/qemu/sockets.h | 4 ++--
 migration.c            | 2 +-
 nbd.c                  | 8 ++++----
 net/socket.c           | 6 +++---
 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    | 4 ++--
 14 files changed, 27 insertions(+), 27 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 7fb2147..3b4b467 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 6c3752b..b5c8e65 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(;;) {
diff --git a/qemu-char.c b/qemu-char.c
index 936150f..500a582 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2564,7 +2564,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;
@@ -2716,7 +2716,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;
 }
@@ -2758,7 +2758,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);
@@ -3650,7 +3650,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 406caa9..b1d8988 100644
--- a/savevm.c
+++ b/savevm.c
@@ -422,7 +422,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 b6b78f5..b632a74 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));
-- 
1.8.1.4

[Qemu-devel] [PULL 2/6] net: ensure "socket" backend uses non-blocking fds

[Luiz Capitulino]

From: Stefan Hajnoczi <stefanha@redhat.com>

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 net/socket.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/socket.c b/net/socket.c
index b5c8e65..87af1d3 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;
     }
+    qemu_set_nonblock(fd);
 
     s = net_socket_fd_init(peer, model, name, fd, 0);
     if (!s) {
@@ -712,7 +713,11 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
         int fd;
 
         fd = monitor_handle_fd_param(cur_mon, sock->fd);
-        if (fd == -1 || !net_socket_fd_init(peer, "socket", name, fd, 1)) {
+        if (fd == -1) {
+            return -1;
+        }
+        qemu_set_nonblock(fd);
+        if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
             return -1;
         }
         return 0;
-- 
1.8.1.4

[Qemu-devel] [PULL 3/6] qemu-socket: set passed fd non-blocking in socket_connect()

[Luiz Capitulino]

From: Stefan Hajnoczi <stefanha@redhat.com>

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@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 b632a74..94581aa 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) {
+            qemu_set_nonblock(fd);
             callback(fd, opaque);
         }
         break;
-- 
1.8.1.4

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

[Luiz Capitulino]

From: Stefan Hajnoczi <stefanha@redhat.com>

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-char.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 500a582..091f2dc 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2434,6 +2434,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

[Qemu-devel] [PULL 5/6] qstring: add qobject_get_length()

[Luiz Capitulino]

Long overdue.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 include/qapi/qmp/qstring.h | 1 +
 qobject/qstring.c          | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 0e690f4..1bc3666 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -26,6 +26,7 @@ typedef struct QString {
 QString *qstring_new(void);
 QString *qstring_from_str(const char *str);
 QString *qstring_from_substr(const char *str, int start, int end);
+size_t qstring_get_length(const QString *qstring);
 const char *qstring_get_str(const QString *qstring);
 void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 5f7376c..607b7a1 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -32,6 +32,14 @@ QString *qstring_new(void)
 }
 
 /**
+ * qstring_get_length(): Get the length of a QString
+ */
+size_t qstring_get_length(const QString *qstring)
+{
+    return qstring->length;
+}
+
+/**
  * qstring_from_substr(): Create a new QString from a C string substring
  *
  * Return string reference
-- 
1.8.1.4

[Qemu-devel] [PULL 6/6] Monitor: Make output buffer dynamic

[Luiz Capitulino]

Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
to retry on qemu_chr_fe_write() errors. However, the Monitor's output
buffer can keep growing while the retry is not issued and this can
cause the buffer to overflow.

To reproduce this issue, just start qemu and type on the Monitor:

(qemu) ?

This will cause the assertion to trig.

To fix this problem this commit makes the Monitor buffer dynamic,
which means that it can grow as much as needed.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5dfae2a..4da45c0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -188,8 +188,7 @@ struct Monitor {
     int reset_seen;
     int flags;
     int suspend_cnt;
-    uint8_t outbuf[1024];
-    int outbuf_index;
+    QString *outbuf;
     ReadLineState *rs;
     MonitorControl *mc;
     CPUArchState *mon_cpu;
@@ -271,45 +270,52 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
 void monitor_flush(Monitor *mon)
 {
     int rc;
+    size_t len;
+    const char *buf;
+
+    buf = qstring_get_str(mon->outbuf);
+    len = qstring_get_length(mon->outbuf);
 
-    if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
-        rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index);
-        if (rc == mon->outbuf_index) {
+    if (mon && len && !mon->mux_out) {
+        rc = qemu_chr_fe_write(mon->chr, (const uint8_t *) buf, len);
+        if (rc == len) {
             /* all flushed */
-            mon->outbuf_index = 0;
+            QDECREF(mon->outbuf);
+            mon->outbuf = qstring_new();
             return;
         }
         if (rc > 0) {
             /* partinal write */
-            memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - rc);
-            mon->outbuf_index -= rc;
+            QString *tmp = qstring_from_str(buf + rc);
+            QDECREF(mon->outbuf);
+            mon->outbuf = tmp;
         }
         qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon);
     }
 }
 
-/* flush at every end of line or if the buffer is full */
+/* flush at every end of line */
 static void monitor_puts(Monitor *mon, const char *str)
 {
     char c;
 
     for(;;) {
-        assert(mon->outbuf_index < sizeof(mon->outbuf) - 1);
         c = *str++;
         if (c == '\0')
             break;
-        if (c == '\n')
-            mon->outbuf[mon->outbuf_index++] = '\r';
-        mon->outbuf[mon->outbuf_index++] = c;
-        if (mon->outbuf_index >= (sizeof(mon->outbuf) - 1)
-            || c == '\n')
+        if (c == '\n') {
+            qstring_append_chr(mon->outbuf, '\r');
+        }
+        qstring_append_chr(mon->outbuf, c);
+        if (c == '\n') {
             monitor_flush(mon);
+        }
     }
 }
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
-    char buf[4096];
+    char *buf;
 
     if (!mon)
         return;
@@ -318,8 +324,9 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
         return;
     }
 
-    vsnprintf(buf, sizeof(buf), fmt, ap);
+    buf = g_strdup_vprintf(fmt, ap);
     monitor_puts(mon, buf);
+    g_free(buf);
 }
 
 void monitor_printf(Monitor *mon, const char *fmt, ...)
@@ -4740,6 +4747,7 @@ void monitor_init(CharDriverState *chr, int flags)
     }
 
     mon = g_malloc0(sizeof(*mon));
+    mon->outbuf = qstring_new();
 
     mon->chr = chr;
     mon->flags = flags;
-- 
1.8.1.4

Re: [Qemu-devel] [PULL 6/6] Monitor: Make output buffer dynamic

[Anthony Liguori]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
> to retry on qemu_chr_fe_write() errors. However, the Monitor's output
> buffer can keep growing while the retry is not issued and this can
> cause the buffer to overflow.
>
> To reproduce this issue, just start qemu and type on the Monitor:
>
> (qemu) ?
>
> This will cause the assertion to trig.
>
> To fix this problem this commit makes the Monitor buffer dynamic,
> which means that it can grow as much as needed.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

This breaks hotplug according to git bisect.  The test output is:

[aliguori@ccnode4 qemu-test]$ QEMU_TEST_SEED=6381 ./qemu-test ~/build/qemu/x86_64-softmmu/qemu-system-x86_64 tests/device-add.sh
Using RANDOM seed 6381
Formatting '.tmp-13653/disk.img', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off 
/home/aliguori/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel /usr/local/share/qemu-jeos/kernel-x86_64-pc -initrd .tmp-13653/initramfs-13653.img.gz -device isa-debug-exit -append console=ttyS0 seed=6381 -nographic -enable-kvm -pidfile .tmp-13653/pidfile-13653.pid -qmp unix:.tmp-13653/qmpsock-13653.sock,server,nowait
<snip>
[    0.863523] Freeing unused kernel memory: 1724k freed
Setting guest RANDOM seed to 6381
*** Running tests ***
/tests/device-add.sh
Running test /tests/device-add.sh...
** waiting for hotplug **
Traceback (most recent call last):
  File "../..//QMP/qmp", line 126, in <module>
    sys.exit(main(sys.argv[1:]))
  File "../..//QMP/qmp", line 122, in main
    rsp = do_command(srv, command, **arguments)
  File "../..//QMP/qmp", line 81, in do_command
./qemu-test: line 99: 13685 Segmentation fault      "$@"
AttributeError: 'NoneType' object has no attribute 'has_key'
./tests/device-add.sh: line 22: test: !=: unary operator expected
Traceback (most recent call last):
  File "../..//QMP/qmp", line 126, in <module>
    sys.exit(main(sys.argv[1:]))
  File "../..//QMP/qmp", line 77, in main
    srv.connect()
  File "/home/aliguori/git/qemu-test/QMP/qmp.py", line 84, in connect
    self.__sock.connect(self.__address)
  File "/usr/lib64/python2.7/socket.py", line 224, in meth
    return getattr(self._sock,name)(*args)
socket.error: [Errno 111] Connection refused
** waiting for guest to see device **

The test is failing because QEMU is SEGV'ing.  The backtrace from the
core is:

Core was generated by `/home/aliguori/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel /usr/local/'.
Program terminated with signal 11, Segmentation fault.
#0  capacity_increase (qstring=qstring@entry=0x0, len=len@entry=1)
    at /home/aliguori/git/qemu/qobject/qstring.c:77
77	    if (qstring->capacity < (qstring->length + len)) {
Missing separate debuginfos, use: debuginfo-install bzip2-libs-1.0.6-7.fc18.x86_64 glib2-2.34.2-2.fc18.x86_64 glibc-2.16-28.fc18.x86_64 libaio-0.3.109-6.fc18.x86_64 libfdt-1.3.0-5.fc18.x86_64 libgcc-4.7.2-8.fc18.x86_64 pixman-0.26.2-5.fc18.x86_64 xen-libs-4.2.1-5.fc18.x86_64 xz-libs-5.1.2-2alpha.fc18.x86_64 zlib-1.2.7-9.fc18.x86_64
(gdb) bt
#0  capacity_increase (qstring=qstring@entry=0x0, len=len@entry=1)
    at /home/aliguori/git/qemu/qobject/qstring.c:77
#1  0x00007f610c43e32d in qstring_append_chr (qstring=0x0, c=79)
    at /home/aliguori/git/qemu/qobject/qstring.c:110
#2  0x00007f610c398357 in monitor_puts (mon=mon@entry=0x7fff2d5d7040, str=
    0x7f610f064c01 "K\n", str@entry=0x7f610f064c00 "OK\n")
    at /home/aliguori/git/qemu/monitor.c:309
#3  0x00007f610c399379 in monitor_vprintf (ap=<optimized out>, 
    fmt=<optimized out>, mon=0x7fff2d5d7040)
    at /home/aliguori/git/qemu/monitor.c:328
#4  monitor_vprintf (mon=0x7fff2d5d7040, fmt=<optimized out>, 
    ap=<optimized out>) at /home/aliguori/git/qemu/monitor.c:316
#5  0x00007f610c399504 in monitor_printf (mon=<optimized out>, 
    fmt=<optimized out>) at /home/aliguori/git/qemu/monitor.c:336
#6  0x00007f610c39e809 in handle_user_command (mon=mon@entry=0x7fff2d5d7040, 
    cmdline=cmdline@entry=
    0x7f610f064530 "drive_add auto file=.tmp-21330/disk.img,if=none,id=hd0")
    at /home/aliguori/git/qemu/monitor.c:4005
#7  0x00007f610c39e9da in qmp_human_monitor_command (command_line=
    0x7f610f064530 "drive_add auto file=.tmp-21330/disk.img,if=none,id=hd0", 
    has_cpu_index=false, cpu_index=140054840363424, errp=errp@entry=
    0x7fff2d5d71c8) at /home/aliguori/git/qemu/monitor.c:697
#8  0x00007f610c2ff969 in qmp_marshal_input_human_monitor_command (
    mon=<optimized out>, qdict=<optimized out>, ret=0x7fff2d5d7250)
    at qmp-marshal.c:1624
#9  0x00007f610c398ff8 in qmp_call_cmd (cmd=<optimized out>, params=
    0x7f610f023980, mon=0x7f610efbb220)
    at /home/aliguori/git/qemu/monitor.c:4506
#10 handle_qmp_command (parser=<optimized out>, tokens=<optimized out>)
    at /home/aliguori/git/qemu/monitor.c:4572
#11 0x00007f610c44051a in json_message_process_token (lexer=0x7f610efbb2c0, 
    token=0x7f610f064320, type=JSON_OPERATOR, x=156, y=0)
    at /home/aliguori/git/qemu/qobject/json-streamer.c:87
#12 0x00007f610c45211f in json_lexer_feed_char (lexer=lexer@entry=
    0x7f610efbb2c0, ch=125 '}', flush=flush@entry=false)
    at /home/aliguori/git/qemu/qobject/json-lexer.c:303
#13 0x00007f610c452266 in json_lexer_feed (lexer=0x7f610efbb2c0, 
    buffer=<optimized out>, size=<optimized out>)
    at /home/aliguori/git/qemu/qobject/json-lexer.c:356
#14 0x00007f610c440731 in json_message_parser_feed (parser=<optimized out>, 
    buffer=<optimized out>, size=<optimized out>)
    at /home/aliguori/git/qemu/qobject/json-streamer.c:110
#15 0x00007f610c3974ab in monitor_control_read (opaque=<optimized out>, 
    buf=<optimized out>, size=<optimized out>)
    at /home/aliguori/git/qemu/monitor.c:4593
#16 0x00007f610c2f6d29 in qemu_chr_be_write (len=<optimized out>, buf=
    0x7fff2d5d7440 "}", s=0x7f610efb2640)
    at /home/aliguori/git/qemu/qemu-char.c:187
#17 tcp_chr_read (chan=<optimized out>, cond=<optimized out>, opaque=
    0x7f610efb2640) at /home/aliguori/git/qemu/qemu-char.c:2530
#18 0x00007f610b83fa55 in g_main_context_dispatch ()
   from /lib64/libglib-2.0.so.0
#19 0x00007f610c2cfe39 in glib_pollfds_poll ()
    at /home/aliguori/git/qemu/main-loop.c:187
#20 os_host_main_loop_wait (timeout=<optimized out>)
    at /home/aliguori/git/qemu/main-loop.c:207
#21 main_loop_wait (nonblocking=<optimized out>)
    at /home/aliguori/git/qemu/main-loop.c:443
#22 0x00007f610c1c4385 in main_loop () at /home/aliguori/git/qemu/vl.c:2043
#23 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
    at /home/aliguori/git/qemu/vl.c:4429

The script for the test is at:

http://git.qemu.org/?p=qemu-test.git;a=blob;f=tests/device-add.sh;h=52ffbf0e10a546d15a2108bba46d97215103b34a;hb=HEAD

The failure is 100% reproducible so if you have any trouble reproducing,
let me know.

Regards,

Anthony Liguori

> ---
>  monitor.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 5dfae2a..4da45c0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -188,8 +188,7 @@ struct Monitor {
>      int reset_seen;
>      int flags;
>      int suspend_cnt;
> -    uint8_t outbuf[1024];
> -    int outbuf_index;
> +    QString *outbuf;
>      ReadLineState *rs;
>      MonitorControl *mc;
>      CPUArchState *mon_cpu;
> @@ -271,45 +270,52 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>  void monitor_flush(Monitor *mon)
>  {
>      int rc;
> +    size_t len;
> +    const char *buf;
> +
> +    buf = qstring_get_str(mon->outbuf);
> +    len = qstring_get_length(mon->outbuf);
>  
> -    if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
> -        rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index);
> -        if (rc == mon->outbuf_index) {
> +    if (mon && len && !mon->mux_out) {
> +        rc = qemu_chr_fe_write(mon->chr, (const uint8_t *) buf, len);
> +        if (rc == len) {
>              /* all flushed */
> -            mon->outbuf_index = 0;
> +            QDECREF(mon->outbuf);
> +            mon->outbuf = qstring_new();
>              return;
>          }
>          if (rc > 0) {
>              /* partinal write */
> -            memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - rc);
> -            mon->outbuf_index -= rc;
> +            QString *tmp = qstring_from_str(buf + rc);
> +            QDECREF(mon->outbuf);
> +            mon->outbuf = tmp;
>          }
>          qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon);
>      }
>  }
>  
> -/* flush at every end of line or if the buffer is full */
> +/* flush at every end of line */
>  static void monitor_puts(Monitor *mon, const char *str)
>  {
>      char c;
>  
>      for(;;) {
> -        assert(mon->outbuf_index < sizeof(mon->outbuf) - 1);
>          c = *str++;
>          if (c == '\0')
>              break;
> -        if (c == '\n')
> -            mon->outbuf[mon->outbuf_index++] = '\r';
> -        mon->outbuf[mon->outbuf_index++] = c;
> -        if (mon->outbuf_index >= (sizeof(mon->outbuf) - 1)
> -            || c == '\n')
> +        if (c == '\n') {
> +            qstring_append_chr(mon->outbuf, '\r');
> +        }
> +        qstring_append_chr(mon->outbuf, c);
> +        if (c == '\n') {
>              monitor_flush(mon);
> +        }
>      }
>  }
>  
>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  {
> -    char buf[4096];
> +    char *buf;
>  
>      if (!mon)
>          return;
> @@ -318,8 +324,9 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>          return;
>      }
>  
> -    vsnprintf(buf, sizeof(buf), fmt, ap);
> +    buf = g_strdup_vprintf(fmt, ap);
>      monitor_puts(mon, buf);
> +    g_free(buf);
>  }
>  
>  void monitor_printf(Monitor *mon, const char *fmt, ...)
> @@ -4740,6 +4747,7 @@ void monitor_init(CharDriverState *chr, int flags)
>      }
>  
>      mon = g_malloc0(sizeof(*mon));
> +    mon->outbuf = qstring_new();
>  
>      mon->chr = chr;
>      mon->flags = flags;
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PULL 6/6] Monitor: Make output buffer dynamic

[Luiz Capitulino]

On Mon, 01 Apr 2013 10:35:34 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
> > to retry on qemu_chr_fe_write() errors. However, the Monitor's output
> > buffer can keep growing while the retry is not issued and this can
> > cause the buffer to overflow.
> >
> > To reproduce this issue, just start qemu and type on the Monitor:
> >
> > (qemu) ?
> >
> > This will cause the assertion to trig.
> >
> > To fix this problem this commit makes the Monitor buffer dynamic,
> > which means that it can grow as much as needed.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> This breaks hotplug according to git bisect.  The test output is:

I'm trying to reproduce this w/o qemu-test as you suggested on irc, but
what I'm getting is:

# ./qemu-qmp -enable-kvm -qmp unix:./qmp-sock,server,nowait -monitor stdio
QEMU 1.4.50 monitor - type 'help' for more information
(qemu) device_add virtio-blk-pci,drive=hd0
Property 'virtio-blk-pci.drive' can't find value 'hd0'
**
ERROR:/home/lcapitulino/work/src/upstream/qmp-unstable/qom/object.c:1003:object_get_canonical_path: assertion failed: (obj->parent != NULL)

Also happens on master, so I'll bisect that one first...

Re: [Qemu-devel] [PULL 6/6] Monitor: Make output buffer dynamic

[Luiz Capitulino]

On Tue, 2 Apr 2013 10:37:01 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Mon, 01 Apr 2013 10:35:34 -0500
> Anthony Liguori <aliguori@us.ibm.com> wrote:
> 
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> > 
> > > Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
> > > to retry on qemu_chr_fe_write() errors. However, the Monitor's output
> > > buffer can keep growing while the retry is not issued and this can
> > > cause the buffer to overflow.
> > >
> > > To reproduce this issue, just start qemu and type on the Monitor:
> > >
> > > (qemu) ?
> > >
> > > This will cause the assertion to trig.
> > >
> > > To fix this problem this commit makes the Monitor buffer dynamic,
> > > which means that it can grow as much as needed.
> > >
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > 
> > This breaks hotplug according to git bisect.  The test output is:
> 
> I'm trying to reproduce this w/o qemu-test as you suggested on irc, but
> what I'm getting is:
> 
> # ./qemu-qmp -enable-kvm -qmp unix:./qmp-sock,server,nowait -monitor stdio
> QEMU 1.4.50 monitor - type 'help' for more information
> (qemu) device_add virtio-blk-pci,drive=hd0
> Property 'virtio-blk-pci.drive' can't find value 'hd0'
> **
> ERROR:/home/lcapitulino/work/src/upstream/qmp-unstable/qom/object.c:1003:object_get_canonical_path: assertion failed: (obj->parent != NULL)
> 
> Also happens on master, so I'll bisect that one first...

I was obviously missing a drive_add first, but the assertion is a bug
anyway.

Re: [Qemu-devel] [PULL 6/6] Monitor: Make output buffer dynamic

[Anthony Liguori]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 2 Apr 2013 10:37:01 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
>> On Mon, 01 Apr 2013 10:35:34 -0500
>> Anthony Liguori <aliguori@us.ibm.com> wrote:
>> 
>> > Luiz Capitulino <lcapitulino@redhat.com> writes:
>> > 
>> > > Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
>> > > to retry on qemu_chr_fe_write() errors. However, the Monitor's output
>> > > buffer can keep growing while the retry is not issued and this can
>> > > cause the buffer to overflow.
>> > >
>> > > To reproduce this issue, just start qemu and type on the Monitor:
>> > >
>> > > (qemu) ?
>> > >
>> > > This will cause the assertion to trig.
>> > >
>> > > To fix this problem this commit makes the Monitor buffer dynamic,
>> > > which means that it can grow as much as needed.
>> > >
>> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > 
>> > This breaks hotplug according to git bisect.  The test output is:
>> 
>> I'm trying to reproduce this w/o qemu-test as you suggested on irc, but
>> what I'm getting is:
>> 
>> # ./qemu-qmp -enable-kvm -qmp unix:./qmp-sock,server,nowait -monitor stdio
>> QEMU 1.4.50 monitor - type 'help' for more information
>> (qemu) device_add virtio-blk-pci,drive=hd0
>> Property 'virtio-blk-pci.drive' can't find value 'hd0'
>> **
>> ERROR:/home/lcapitulino/work/src/upstream/qmp-unstable/qom/object.c:1003:object_get_canonical_path: assertion failed: (obj->parent != NULL)
>> 
>> Also happens on master, so I'll bisect that one first...
>
> I was obviously missing a drive_add first, but the assertion is a bug
> anyway.

I've got a patch waiting to be pushed that fixes the assert.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PULL 6/6] Monitor: Make output buffer dynamic

[Luiz Capitulino]

On Tue, 02 Apr 2013 10:50:49 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Tue, 2 Apr 2013 10:37:01 -0400
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >
> >> On Mon, 01 Apr 2013 10:35:34 -0500
> >> Anthony Liguori <aliguori@us.ibm.com> wrote:
> >> 
> >> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> > 
> >> > > Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
> >> > > to retry on qemu_chr_fe_write() errors. However, the Monitor's output
> >> > > buffer can keep growing while the retry is not issued and this can
> >> > > cause the buffer to overflow.
> >> > >
> >> > > To reproduce this issue, just start qemu and type on the Monitor:
> >> > >
> >> > > (qemu) ?
> >> > >
> >> > > This will cause the assertion to trig.
> >> > >
> >> > > To fix this problem this commit makes the Monitor buffer dynamic,
> >> > > which means that it can grow as much as needed.
> >> > >
> >> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> > 
> >> > This breaks hotplug according to git bisect.  The test output is:
> >> 
> >> I'm trying to reproduce this w/o qemu-test as you suggested on irc, but
> >> what I'm getting is:
> >> 
> >> # ./qemu-qmp -enable-kvm -qmp unix:./qmp-sock,server,nowait -monitor stdio
> >> QEMU 1.4.50 monitor - type 'help' for more information
> >> (qemu) device_add virtio-blk-pci,drive=hd0
> >> Property 'virtio-blk-pci.drive' can't find value 'hd0'
> >> **
> >> ERROR:/home/lcapitulino/work/src/upstream/qmp-unstable/qom/object.c:1003:object_get_canonical_path: assertion failed: (obj->parent != NULL)
> >> 
> >> Also happens on master, so I'll bisect that one first...
> >
> > I was obviously missing a drive_add first, but the assertion is a bug
> > anyway.
> 
> I've got a patch waiting to be pushed that fixes the assert.

Thanks. The monitor bug you got happens because qmp_human_monitor_command()
doesn't initialize the dynamic buffer. I'll respin the series.