[Qemu-devel] [PATCH v2] vhost-user: don't poke at chardev internal QemuOpts

[Qemu-devel] [PATCH v2] vhost-user: don't poke at chardev internal QemuOpts

[Daniel P. Berrange]

The vhost-user code is poking at the QemuOpts instance
in the CharDriverState struct, not realizing that it is
valid for this to be NULL. e.g. the following crash
shows a codepath where it will be NULL:

 Program terminated with signal SIGSEGV, Segmentation fault.
 #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at util/qemu-option.c:617
 617         QTAILQ_FOREACH(opt, &opts->head, next) {
 [Current thread is 1 (Thread 0x7f1d4970bb40 (LWP 6603))]
 (gdb) bt
 #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at util/qemu-option.c:617
 #1  0x000055baf696b7da in net_vhost_parse_chardev (opts=0x55baf8ff9260, errp=0x7ffc51368e48) at net/vhost-user.c:314
 #2  0x000055baf696b985 in net_init_vhost_user (netdev=0x55baf8ff9250, name=0x55baf879d270 "hostnet2", peer=0x0, errp=0x7ffc51368e48) at net/vhost-user.c:360
 #3  0x000055baf6960216 in net_client_init1 (object=0x55baf8ff9250, is_netdev=true, errp=0x7ffc51368e48) at net/net.c:1051
 #4  0x000055baf6960518 in net_client_init (opts=0x55baf776e7e0, is_netdev=true, errp=0x7ffc51368f00) at net/net.c:1108
 #5  0x000055baf696083f in netdev_add (opts=0x55baf776e7e0, errp=0x7ffc51368f00) at net/net.c:1186
 #6  0x000055baf69608c7 in qmp_netdev_add (qdict=0x55baf7afaf60, ret=0x7ffc51368f50, errp=0x7ffc51368f48) at net/net.c:1205
 #7  0x000055baf6622135 in handle_qmp_command (parser=0x55baf77fb590, tokens=0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978
 #8  0x000055baf6a9d099 in json_message_process_token (lexer=0x55baf77fb598, input=0x55baf75acd20, type=JSON_RCURLY, x=113, y=19) at qobject/json-streamer.c:105
 #9  0x000055baf6abf7aa in json_lexer_feed_char (lexer=0x55baf77fb598, ch=125 '}', flush=false) at qobject/json-lexer.c:319
 #10 0x000055baf6abf8f2 in json_lexer_feed (lexer=0x55baf77fb598, buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-lexer.c:369
 #11 0x000055baf6a9d13c in json_message_parser_feed (parser=0x55baf77fb590, buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-streamer.c:124
 #12 0x000055baf66221f7 in monitor_qmp_read (opaque=0x55baf77fb530, buf=0x7ffc51369170 "}R\204\367\272U", size=1) at /path/to/qemu.git/monitor.c:3994
 #13 0x000055baf6757014 in qemu_chr_be_write_impl (s=0x55baf7610a40, buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:387
 #14 0x000055baf6757076 in qemu_chr_be_write (s=0x55baf7610a40, buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:399
 #15 0x000055baf675b3b0 in tcp_chr_read (chan=0x55baf90244b0, cond=G_IO_IN, opaque=0x55baf7610a40) at qemu-char.c:2927
 #16 0x000055baf6a5d655 in qio_channel_fd_source_dispatch (source=0x55baf7610df0, callback=0x55baf675b25a <tcp_chr_read>, user_data=0x55baf7610a40) at io/channel-watch.c:84
 #17 0x00007f1d3e80cbbd in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
 #18 0x000055baf69d3720 in glib_pollfds_poll () at main-loop.c:213
 #19 0x000055baf69d37fd in os_host_main_loop_wait (timeout=126000000) at main-loop.c:258
 #20 0x000055baf69d38ad in main_loop_wait (nonblocking=0) at main-loop.c:506
 #21 0x000055baf676587b in main_loop () at vl.c:1908
 #22 0x000055baf676d3bf in main (argc=101, argv=0x7ffc5136a6c8, envp=0x7ffc5136a9f8) at vl.c:4604
 (gdb) p opts
 $1 = (QemuOpts *) 0x0

The crash occurred when attaching vhost-user net via QMP:

{
    "execute": "chardev-add",
    "arguments": {
        "id": "charnet2",
        "backend": {
            "type": "socket",
            "data": {
                "addr": {
                    "type": "unix",
                    "data": {
                        "path": "/var/run/openvswitch/vhost-user1"
                    }
                },
                "wait": false,
                "server": false
            }
        }
    },
    "id": "libvirt-19"
}
{
    "return": {

    },
    "id": "libvirt-19"
}
{
    "execute": "netdev_add",
    "arguments": {
        "type": "vhost-user",
        "chardev": "charnet2",
        "id": "hostnet2"
    },
    "id": "libvirt-20"
}

The vhost-user code should not be poking at the internals of the
CharDriverState struct. What it wants is a chardev that is operating
as a network server, along with the ability to do FD passing over
the connection. Add a feature concept to the char drivers so that
vhost-user can query the actual features it wishes to have supported.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---

Changed in v2:

 - Switch to use a bitmap to track features in chardev
 - Check for FD passing support in chardev

 include/sysemu/char.h | 19 +++++++++++++++++++
 net/vhost-user.c      | 41 +++++++----------------------------------
 qemu-char.c           | 20 ++++++++++++++++++++
 3 files changed, 46 insertions(+), 34 deletions(-)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 0d0465a..35abeec 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -9,6 +9,7 @@
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qstring.h"
 #include "qemu/main-loop.h"
+#include "qemu/bitmap.h"
 
 /* character device */
 
@@ -58,6 +59,19 @@ struct ParallelIOArg {
 
 typedef void IOEventHandler(void *opaque, int event);
 
+typedef enum {
+    /* Whether the chardev acts as a network server and can
+     * thus support qemu_chr_wait_connected() to wait for
+     * incoming clients */
+    QEMU_CHAR_FEATURE_NETWORK_SERVER,
+    /* Whether it is possible to send/recv file descriptors
+     * over the data channel */
+    QEMU_CHAR_FEATURE_FD_PASS,
+
+    QEMU_CHAR_FEATURE_LAST,
+} CharDriverFeature;
+
+
 struct CharDriverState {
     QemuMutex chr_write_lock;
     void (*init)(struct CharDriverState *s);
@@ -95,6 +109,7 @@ struct CharDriverState {
     guint fd_in_tag;
     QemuOpts *opts;
     bool replay;
+    DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
@@ -437,6 +452,10 @@ int qemu_chr_add_client(CharDriverState *s, int fd);
 CharDriverState *qemu_chr_find(const char *name);
 bool chr_is_ringbuf(const CharDriverState *chr);
 
+bool qemu_chr_has_feature(CharDriverState *chr,
+                          CharDriverFeature feature);
+void qemu_chr_set_feature(CharDriverState *chr,
+                          CharDriverFeature feature);
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
 
 void register_char_driver(const char *name, ChardevBackendKind kind,
diff --git a/net/vhost-user.c b/net/vhost-user.c
index b0595f8..13f553b 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -27,11 +27,6 @@ typedef struct VhostUserState {
     bool started;
 } VhostUserState;
 
-typedef struct VhostUserChardevProps {
-    bool is_socket;
-    bool is_unix;
-} VhostUserChardevProps;
-
 VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
 {
     VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
@@ -278,45 +273,23 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
     return 0;
 }
 
-static int net_vhost_chardev_opts(void *opaque,
-                                  const char *name, const char *value,
-                                  Error **errp)
-{
-    VhostUserChardevProps *props = opaque;
-
-    if (strcmp(name, "backend") == 0 && strcmp(value, "socket") == 0) {
-        props->is_socket = true;
-    } else if (strcmp(name, "path") == 0) {
-        props->is_unix = true;
-    } else if (strcmp(name, "server") == 0) {
-    } else {
-        error_setg(errp,
-                   "vhost-user does not support a chardev with option %s=%s",
-                   name, value);
-        return -1;
-    }
-    return 0;
-}
-
-static CharDriverState *net_vhost_parse_chardev(
+static CharDriverState *net_vhost_claim_chardev(
     const NetdevVhostUserOptions *opts, Error **errp)
 {
     CharDriverState *chr = qemu_chr_find(opts->chardev);
-    VhostUserChardevProps props;
 
     if (chr == NULL) {
         error_setg(errp, "chardev \"%s\" not found", opts->chardev);
         return NULL;
     }
 
-    /* inspect chardev opts */
-    memset(&props, 0, sizeof(props));
-    if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props, errp)) {
+    if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_NETWORK_SERVER)) {
+        error_setg(errp, "chardev \"%s\" does not provide a network server",
+                   opts->chardev);
         return NULL;
     }
-
-    if (!props.is_socket || !props.is_unix) {
-        error_setg(errp, "chardev \"%s\" is not a unix socket",
+    if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_FD_PASS)) {
+        error_setg(errp, "chardev \"%s\" does not support FD passing",
                    opts->chardev);
         return NULL;
     }
@@ -357,7 +330,7 @@ int net_init_vhost_user(const Netdev *netdev, const char *name,
     assert(netdev->type == NET_CLIENT_DRIVER_VHOST_USER);
     vhost_user_opts = &netdev->u.vhost_user;
 
-    chr = net_vhost_parse_chardev(vhost_user_opts, errp);
+    chr = net_vhost_claim_chardev(vhost_user_opts, errp);
     if (!chr) {
         return -1;
     }
diff --git a/qemu-char.c b/qemu-char.c
index fb456ce..92eb0d5 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4513,6 +4513,13 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
 
     s->addr = QAPI_CLONE(SocketAddress, sock->addr);
 
+    if (is_listen) {
+        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_NETWORK_SERVER);
+    }
+    if (s->is_unix) {
+        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
+    }
+
     chr->opaque = s;
     chr->chr_wait_connected = tcp_chr_wait_connected;
     chr->chr_write = tcp_chr_write;
@@ -4596,6 +4603,19 @@ static CharDriverState *qmp_chardev_open_udp(const char *id,
     return qemu_chr_open_udp(sioc, common, errp);
 }
 
+
+bool qemu_chr_has_feature(CharDriverState *chr,
+                          CharDriverFeature feature)
+{
+    return test_bit(feature, chr->features);
+}
+
+void qemu_chr_set_feature(CharDriverState *chr,
+                           CharDriverFeature feature)
+{
+    return set_bit(feature, chr->features);
+}
+
 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
                                Error **errp)
 {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] vhost-user: don't poke at chardev internal QemuOpts

[Peter Maydell]

On 7 October 2016 at 10:49, Daniel P. Berrange <berrange@redhat.com> wrote:
> The vhost-user code is poking at the QemuOpts instance
> in the CharDriverState struct, not realizing that it is
> valid for this to be NULL. e.g. the following crash
> shows a codepath where it will be NULL:

> +typedef enum {
> +    /* Whether the chardev acts as a network server and can
> +     * thus support qemu_chr_wait_connected() to wait for
> +     * incoming clients */
> +    QEMU_CHAR_FEATURE_NETWORK_SERVER,
> +    /* Whether it is possible to send/recv file descriptors
> +     * over the data channel */
> +    QEMU_CHAR_FEATURE_FD_PASS,
> +
> +    QEMU_CHAR_FEATURE_LAST,
> +} CharDriverFeature;

Will net/colo-compare.c need more features than this, or
will these suffice for both?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] vhost-user: don't poke at chardev internal QemuOpts

[Marc-André Lureau]

Hi

On Fri, Oct 7, 2016 at 1:50 PM Daniel P. Berrange <berrange@redhat.com>
wrote:

> The vhost-user code is poking at the QemuOpts instance
> in the CharDriverState struct, not realizing that it is
> valid for this to be NULL. e.g. the following crash
> shows a codepath where it will be NULL:
>
>  Program terminated with signal SIGSEGV, Segmentation fault.
>  #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650
> <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at
> util/qemu-option.c:617
>  617         QTAILQ_FOREACH(opt, &opts->head, next) {
>  [Current thread is 1 (Thread 0x7f1d4970bb40 (LWP 6603))]
>  (gdb) bt
>  #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650
> <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at
> util/qemu-option.c:617
>  #1  0x000055baf696b7da in net_vhost_parse_chardev (opts=0x55baf8ff9260,
> errp=0x7ffc51368e48) at net/vhost-user.c:314
>  #2  0x000055baf696b985 in net_init_vhost_user (netdev=0x55baf8ff9250,
> name=0x55baf879d270 "hostnet2", peer=0x0, errp=0x7ffc51368e48) at
> net/vhost-user.c:360
>  #3  0x000055baf6960216 in net_client_init1 (object=0x55baf8ff9250,
> is_netdev=true, errp=0x7ffc51368e48) at net/net.c:1051
>  #4  0x000055baf6960518 in net_client_init (opts=0x55baf776e7e0,
> is_netdev=true, errp=0x7ffc51368f00) at net/net.c:1108
>  #5  0x000055baf696083f in netdev_add (opts=0x55baf776e7e0,
> errp=0x7ffc51368f00) at net/net.c:1186
>  #6  0x000055baf69608c7 in qmp_netdev_add (qdict=0x55baf7afaf60,
> ret=0x7ffc51368f50, errp=0x7ffc51368f48) at net/net.c:1205
>  #7  0x000055baf6622135 in handle_qmp_command (parser=0x55baf77fb590,
> tokens=0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978
>  #8  0x000055baf6a9d099 in json_message_process_token
> (lexer=0x55baf77fb598, input=0x55baf75acd20, type=JSON_RCURLY, x=113, y=19)
> at qobject/json-streamer.c:105
>  #9  0x000055baf6abf7aa in json_lexer_feed_char (lexer=0x55baf77fb598,
> ch=125 '}', flush=false) at qobject/json-lexer.c:319
>  #10 0x000055baf6abf8f2 in json_lexer_feed (lexer=0x55baf77fb598,
> buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-lexer.c:369
>  #11 0x000055baf6a9d13c in json_message_parser_feed
> (parser=0x55baf77fb590, buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at
> qobject/json-streamer.c:124
>  #12 0x000055baf66221f7 in monitor_qmp_read (opaque=0x55baf77fb530,
> buf=0x7ffc51369170 "}R\204\367\272U", size=1) at
> /path/to/qemu.git/monitor.c:3994
>  #13 0x000055baf6757014 in qemu_chr_be_write_impl (s=0x55baf7610a40,
> buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:387
>  #14 0x000055baf6757076 in qemu_chr_be_write (s=0x55baf7610a40,
> buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:399
>  #15 0x000055baf675b3b0 in tcp_chr_read (chan=0x55baf90244b0,
> cond=G_IO_IN, opaque=0x55baf7610a40) at qemu-char.c:2927
>  #16 0x000055baf6a5d655 in qio_channel_fd_source_dispatch
> (source=0x55baf7610df0, callback=0x55baf675b25a <tcp_chr_read>,
> user_data=0x55baf7610a40) at io/channel-watch.c:84
>  #17 0x00007f1d3e80cbbd in g_main_context_dispatch () from
> /usr/lib64/libglib-2.0.so.0
>  #18 0x000055baf69d3720 in glib_pollfds_poll () at main-loop.c:213
>  #19 0x000055baf69d37fd in os_host_main_loop_wait (timeout=126000000) at
> main-loop.c:258
>  #20 0x000055baf69d38ad in main_loop_wait (nonblocking=0) at
> main-loop.c:506
>  #21 0x000055baf676587b in main_loop () at vl.c:1908
>  #22 0x000055baf676d3bf in main (argc=101, argv=0x7ffc5136a6c8,
> envp=0x7ffc5136a9f8) at vl.c:4604
>  (gdb) p opts
>  $1 = (QemuOpts *) 0x0
>
> The crash occurred when attaching vhost-user net via QMP:
>
> {
>     "execute": "chardev-add",
>     "arguments": {
>         "id": "charnet2",
>         "backend": {
>             "type": "socket",
>             "data": {
>                 "addr": {
>                     "type": "unix",
>                     "data": {
>                         "path": "/var/run/openvswitch/vhost-user1"
>                     }
>                 },
>                 "wait": false,
>                 "server": false
>             }
>         }
>     },
>     "id": "libvirt-19"
> }
> {
>     "return": {
>
>     },
>     "id": "libvirt-19"
> }
> {
>     "execute": "netdev_add",
>     "arguments": {
>         "type": "vhost-user",
>         "chardev": "charnet2",
>         "id": "hostnet2"
>     },
>     "id": "libvirt-20"
> }
>
> The vhost-user code should not be poking at the internals of the
> CharDriverState struct. What it wants is a chardev that is operating
> as a network server, along with the ability to do FD passing over
> the connection. Add a feature concept to the char drivers so that
> vhost-user can query the actual features it wishes to have supported.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>
> Changed in v2:
>
>  - Switch to use a bitmap to track features in chardev
>  - Check for FD passing support in chardev
>
>  include/sysemu/char.h | 19 +++++++++++++++++++
>  net/vhost-user.c      | 41 +++++++----------------------------------
>  qemu-char.c           | 20 ++++++++++++++++++++
>  3 files changed, 46 insertions(+), 34 deletions(-)
>
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 0d0465a..35abeec 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -9,6 +9,7 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qemu/main-loop.h"
> +#include "qemu/bitmap.h"
>
>  /* character device */
>
> @@ -58,6 +59,19 @@ struct ParallelIOArg {
>
>  typedef void IOEventHandler(void *opaque, int event);
>
> +typedef enum {
> +    /* Whether the chardev acts as a network server and can
> +     * thus support qemu_chr_wait_connected() to wait for
> +     * incoming clients */
> +    QEMU_CHAR_FEATURE_NETWORK_SERVER,
> +    /* Whether it is possible to send/recv file descriptors
> +     * over the data channel */
> +    QEMU_CHAR_FEATURE_FD_PASS,
> +
> +    QEMU_CHAR_FEATURE_LAST,
> +} CharDriverFeature;
> +
> +
>  struct CharDriverState {
>      QemuMutex chr_write_lock;
>      void (*init)(struct CharDriverState *s);
> @@ -95,6 +109,7 @@ struct CharDriverState {
>      guint fd_in_tag;
>      QemuOpts *opts;
>      bool replay;
> +    DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
>
> @@ -437,6 +452,10 @@ int qemu_chr_add_client(CharDriverState *s, int fd);
>  CharDriverState *qemu_chr_find(const char *name);
>  bool chr_is_ringbuf(const CharDriverState *chr);
>
> +bool qemu_chr_has_feature(CharDriverState *chr,
> +                          CharDriverFeature feature);
> +void qemu_chr_set_feature(CharDriverState *chr,
> +                          CharDriverFeature feature);
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
>
>  void register_char_driver(const char *name, ChardevBackendKind kind,
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index b0595f8..13f553b 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -27,11 +27,6 @@ typedef struct VhostUserState {
>      bool started;
>  } VhostUserState;
>
> -typedef struct VhostUserChardevProps {
> -    bool is_socket;
> -    bool is_unix;
> -} VhostUserChardevProps;
> -
>  VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
>  {
>      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> @@ -278,45 +273,23 @@ static int net_vhost_user_init(NetClientState *peer,
> const char *device,
>      return 0;
>  }
>
> -static int net_vhost_chardev_opts(void *opaque,
> -                                  const char *name, const char *value,
> -                                  Error **errp)
> -{
> -    VhostUserChardevProps *props = opaque;
> -
> -    if (strcmp(name, "backend") == 0 && strcmp(value, "socket") == 0) {
> -        props->is_socket = true;
> -    } else if (strcmp(name, "path") == 0) {
> -        props->is_unix = true;
> -    } else if (strcmp(name, "server") == 0) {
> -    } else {
> -        error_setg(errp,
> -                   "vhost-user does not support a chardev with option
> %s=%s",
> -                   name, value);
> -        return -1;
> -    }
> -    return 0;
> -}
> -
> -static CharDriverState *net_vhost_parse_chardev(
> +static CharDriverState *net_vhost_claim_chardev(
>      const NetdevVhostUserOptions *opts, Error **errp)
>  {
>      CharDriverState *chr = qemu_chr_find(opts->chardev);
> -    VhostUserChardevProps props;
>
>      if (chr == NULL) {
>          error_setg(errp, "chardev \"%s\" not found", opts->chardev);
>          return NULL;
>      }
>
> -    /* inspect chardev opts */
> -    memset(&props, 0, sizeof(props));
> -    if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props,
> errp)) {
> +    if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_NETWORK_SERVER)) {
> +        error_setg(errp, "chardev \"%s\" does not provide a network
> server",
> +                   opts->chardev);
>          return NULL;
>      }
>

It doesn't have to be a server, it can be a client too. Too bad we don't
check that aspect in vhost-user-test already.


> -
> -    if (!props.is_socket || !props.is_unix) {
> -        error_setg(errp, "chardev \"%s\" is not a unix socket",
> +    if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_FD_PASS)) {
> +        error_setg(errp, "chardev \"%s\" does not support FD passing",
>                     opts->chardev);
>          return NULL;
>      }
> @@ -357,7 +330,7 @@ int net_init_vhost_user(const Netdev *netdev, const
> char *name,
>      assert(netdev->type == NET_CLIENT_DRIVER_VHOST_USER);
>      vhost_user_opts = &netdev->u.vhost_user;
>
> -    chr = net_vhost_parse_chardev(vhost_user_opts, errp);
> +    chr = net_vhost_claim_chardev(vhost_user_opts, errp);
>      if (!chr) {
>          return -1;
>      }
> diff --git a/qemu-char.c b/qemu-char.c
> index fb456ce..92eb0d5 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -4513,6 +4513,13 @@ static CharDriverState
> *qmp_chardev_open_socket(const char *id,
>
>      s->addr = QAPI_CLONE(SocketAddress, sock->addr);
>
> +    if (is_listen) {
> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_NETWORK_SERVER);
> +    }
> +    if (s->is_unix) {
> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
> +    }
> +
>      chr->opaque = s;
>      chr->chr_wait_connected = tcp_chr_wait_connected;
>      chr->chr_write = tcp_chr_write;
> @@ -4596,6 +4603,19 @@ static CharDriverState *qmp_chardev_open_udp(const
> char *id,
>      return qemu_chr_open_udp(sioc, common, errp);
>  }
>
> +
> +bool qemu_chr_has_feature(CharDriverState *chr,
> +                          CharDriverFeature feature)
> +{
> +    return test_bit(feature, chr->features);
> +}
> +
> +void qemu_chr_set_feature(CharDriverState *chr,
> +                           CharDriverFeature feature)
> +{
> +    return set_bit(feature, chr->features);
> +}
> +
>  ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
>                                 Error **errp)
>  {
> --
> 2.7.4
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2] vhost-user: don't poke at chardev internal QemuOpts

[Daniel P. Berrange]

On Fri, Oct 07, 2016 at 10:59:09AM +0100, Peter Maydell wrote:
> On 7 October 2016 at 10:49, Daniel P. Berrange <berrange@redhat.com> wrote:
> > The vhost-user code is poking at the QemuOpts instance
> > in the CharDriverState struct, not realizing that it is
> > valid for this to be NULL. e.g. the following crash
> > shows a codepath where it will be NULL:
> 
> > +typedef enum {
> > +    /* Whether the chardev acts as a network server and can
> > +     * thus support qemu_chr_wait_connected() to wait for
> > +     * incoming clients */
> > +    QEMU_CHAR_FEATURE_NETWORK_SERVER,
> > +    /* Whether it is possible to send/recv file descriptors
> > +     * over the data channel */
> > +    QEMU_CHAR_FEATURE_FD_PASS,
> > +
> > +    QEMU_CHAR_FEATURE_LAST,
> > +} CharDriverFeature;
> 
> Will net/colo-compare.c need more features than this, or
> will these suffice for both?

Oh, I didn't notice that colo did the same nasty thing. I'm not actually
seeing why colo wants to force a particular chardev backend - at first
glance it does not appear to be using features that rely on a particular
backend, in the way that vhost-user does. So maybe its sufficient to just
kill the checks in colo.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH v2] vhost-user: don't poke at chardev internal QemuOpts

[Daniel P. Berrange]

On Fri, Oct 07, 2016 at 10:12:23AM +0000, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Oct 7, 2016 at 1:50 PM Daniel P. Berrange <berrange@redhat.com>
> wrote:
> 
> > The vhost-user code is poking at the QemuOpts instance
> > in the CharDriverState struct, not realizing that it is
> > valid for this to be NULL. e.g. the following crash
> > shows a codepath where it will be NULL:
> >
> >  Program terminated with signal SIGSEGV, Segmentation fault.
> >  #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650
> > <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at
> > util/qemu-option.c:617
> >  617         QTAILQ_FOREACH(opt, &opts->head, next) {
> >  [Current thread is 1 (Thread 0x7f1d4970bb40 (LWP 6603))]
> >  (gdb) bt
> >  #0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650
> > <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at
> > util/qemu-option.c:617
> >  #1  0x000055baf696b7da in net_vhost_parse_chardev (opts=0x55baf8ff9260,
> > errp=0x7ffc51368e48) at net/vhost-user.c:314
> >  #2  0x000055baf696b985 in net_init_vhost_user (netdev=0x55baf8ff9250,
> > name=0x55baf879d270 "hostnet2", peer=0x0, errp=0x7ffc51368e48) at
> > net/vhost-user.c:360
> >  #3  0x000055baf6960216 in net_client_init1 (object=0x55baf8ff9250,
> > is_netdev=true, errp=0x7ffc51368e48) at net/net.c:1051
> >  #4  0x000055baf6960518 in net_client_init (opts=0x55baf776e7e0,
> > is_netdev=true, errp=0x7ffc51368f00) at net/net.c:1108
> >  #5  0x000055baf696083f in netdev_add (opts=0x55baf776e7e0,
> > errp=0x7ffc51368f00) at net/net.c:1186
> >  #6  0x000055baf69608c7 in qmp_netdev_add (qdict=0x55baf7afaf60,
> > ret=0x7ffc51368f50, errp=0x7ffc51368f48) at net/net.c:1205
> >  #7  0x000055baf6622135 in handle_qmp_command (parser=0x55baf77fb590,
> > tokens=0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978
> >  #8  0x000055baf6a9d099 in json_message_process_token
> > (lexer=0x55baf77fb598, input=0x55baf75acd20, type=JSON_RCURLY, x=113, y=19)
> > at qobject/json-streamer.c:105
> >  #9  0x000055baf6abf7aa in json_lexer_feed_char (lexer=0x55baf77fb598,
> > ch=125 '}', flush=false) at qobject/json-lexer.c:319
> >  #10 0x000055baf6abf8f2 in json_lexer_feed (lexer=0x55baf77fb598,
> > buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-lexer.c:369
> >  #11 0x000055baf6a9d13c in json_message_parser_feed
> > (parser=0x55baf77fb590, buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at
> > qobject/json-streamer.c:124
> >  #12 0x000055baf66221f7 in monitor_qmp_read (opaque=0x55baf77fb530,
> > buf=0x7ffc51369170 "}R\204\367\272U", size=1) at
> > /path/to/qemu.git/monitor.c:3994
> >  #13 0x000055baf6757014 in qemu_chr_be_write_impl (s=0x55baf7610a40,
> > buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:387
> >  #14 0x000055baf6757076 in qemu_chr_be_write (s=0x55baf7610a40,
> > buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:399
> >  #15 0x000055baf675b3b0 in tcp_chr_read (chan=0x55baf90244b0,
> > cond=G_IO_IN, opaque=0x55baf7610a40) at qemu-char.c:2927
> >  #16 0x000055baf6a5d655 in qio_channel_fd_source_dispatch
> > (source=0x55baf7610df0, callback=0x55baf675b25a <tcp_chr_read>,
> > user_data=0x55baf7610a40) at io/channel-watch.c:84
> >  #17 0x00007f1d3e80cbbd in g_main_context_dispatch () from
> > /usr/lib64/libglib-2.0.so.0
> >  #18 0x000055baf69d3720 in glib_pollfds_poll () at main-loop.c:213
> >  #19 0x000055baf69d37fd in os_host_main_loop_wait (timeout=126000000) at
> > main-loop.c:258
> >  #20 0x000055baf69d38ad in main_loop_wait (nonblocking=0) at
> > main-loop.c:506
> >  #21 0x000055baf676587b in main_loop () at vl.c:1908
> >  #22 0x000055baf676d3bf in main (argc=101, argv=0x7ffc5136a6c8,
> > envp=0x7ffc5136a9f8) at vl.c:4604
> >  (gdb) p opts
> >  $1 = (QemuOpts *) 0x0
> >
> > The crash occurred when attaching vhost-user net via QMP:
> >
> > {
> >     "execute": "chardev-add",
> >     "arguments": {
> >         "id": "charnet2",
> >         "backend": {
> >             "type": "socket",
> >             "data": {
> >                 "addr": {
> >                     "type": "unix",
> >                     "data": {
> >                         "path": "/var/run/openvswitch/vhost-user1"
> >                     }
> >                 },
> >                 "wait": false,
> >                 "server": false
> >             }
> >         }
> >     },
> >     "id": "libvirt-19"
> > }
> > {
> >     "return": {
> >
> >     },
> >     "id": "libvirt-19"
> > }
> > {
> >     "execute": "netdev_add",
> >     "arguments": {
> >         "type": "vhost-user",
> >         "chardev": "charnet2",
> >         "id": "hostnet2"
> >     },
> >     "id": "libvirt-20"
> > }
> >
> > The vhost-user code should not be poking at the internals of the
> > CharDriverState struct. What it wants is a chardev that is operating
> > as a network server, along with the ability to do FD passing over
> > the connection. Add a feature concept to the char drivers so that
> > vhost-user can query the actual features it wishes to have supported.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >
> > Changed in v2:
> >
> >  - Switch to use a bitmap to track features in chardev
> >  - Check for FD passing support in chardev
> >
> >  include/sysemu/char.h | 19 +++++++++++++++++++
> >  net/vhost-user.c      | 41 +++++++----------------------------------
> >  qemu-char.c           | 20 ++++++++++++++++++++
> >  3 files changed, 46 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> > index 0d0465a..35abeec 100644
> > --- a/include/sysemu/char.h
> > +++ b/include/sysemu/char.h
> > @@ -9,6 +9,7 @@
> >  #include "qapi/qmp/qobject.h"
> >  #include "qapi/qmp/qstring.h"
> >  #include "qemu/main-loop.h"
> > +#include "qemu/bitmap.h"
> >
> >  /* character device */
> >
> > @@ -58,6 +59,19 @@ struct ParallelIOArg {
> >
> >  typedef void IOEventHandler(void *opaque, int event);
> >
> > +typedef enum {
> > +    /* Whether the chardev acts as a network server and can
> > +     * thus support qemu_chr_wait_connected() to wait for
> > +     * incoming clients */
> > +    QEMU_CHAR_FEATURE_NETWORK_SERVER,
> > +    /* Whether it is possible to send/recv file descriptors
> > +     * over the data channel */
> > +    QEMU_CHAR_FEATURE_FD_PASS,
> > +
> > +    QEMU_CHAR_FEATURE_LAST,
> > +} CharDriverFeature;
> > +
> > +
> >  struct CharDriverState {
> >      QemuMutex chr_write_lock;
> >      void (*init)(struct CharDriverState *s);
> > @@ -95,6 +109,7 @@ struct CharDriverState {
> >      guint fd_in_tag;
> >      QemuOpts *opts;
> >      bool replay;
> > +    DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
> >      QTAILQ_ENTRY(CharDriverState) next;
> >  };
> >
> > @@ -437,6 +452,10 @@ int qemu_chr_add_client(CharDriverState *s, int fd);
> >  CharDriverState *qemu_chr_find(const char *name);
> >  bool chr_is_ringbuf(const CharDriverState *chr);
> >
> > +bool qemu_chr_has_feature(CharDriverState *chr,
> > +                          CharDriverFeature feature);
> > +void qemu_chr_set_feature(CharDriverState *chr,
> > +                          CharDriverFeature feature);
> >  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
> >
> >  void register_char_driver(const char *name, ChardevBackendKind kind,
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index b0595f8..13f553b 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -27,11 +27,6 @@ typedef struct VhostUserState {
> >      bool started;
> >  } VhostUserState;
> >
> > -typedef struct VhostUserChardevProps {
> > -    bool is_socket;
> > -    bool is_unix;
> > -} VhostUserChardevProps;
> > -
> >  VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> >  {
> >      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> > @@ -278,45 +273,23 @@ static int net_vhost_user_init(NetClientState *peer,
> > const char *device,
> >      return 0;
> >  }
> >
> > -static int net_vhost_chardev_opts(void *opaque,
> > -                                  const char *name, const char *value,
> > -                                  Error **errp)
> > -{
> > -    VhostUserChardevProps *props = opaque;
> > -
> > -    if (strcmp(name, "backend") == 0 && strcmp(value, "socket") == 0) {
> > -        props->is_socket = true;
> > -    } else if (strcmp(name, "path") == 0) {
> > -        props->is_unix = true;
> > -    } else if (strcmp(name, "server") == 0) {
> > -    } else {
> > -        error_setg(errp,
> > -                   "vhost-user does not support a chardev with option
> > %s=%s",
> > -                   name, value);
> > -        return -1;
> > -    }
> > -    return 0;
> > -}
> > -
> > -static CharDriverState *net_vhost_parse_chardev(
> > +static CharDriverState *net_vhost_claim_chardev(
> >      const NetdevVhostUserOptions *opts, Error **errp)
> >  {
> >      CharDriverState *chr = qemu_chr_find(opts->chardev);
> > -    VhostUserChardevProps props;
> >
> >      if (chr == NULL) {
> >          error_setg(errp, "chardev \"%s\" not found", opts->chardev);
> >          return NULL;
> >      }
> >
> > -    /* inspect chardev opts */
> > -    memset(&props, 0, sizeof(props));
> > -    if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props,
> > errp)) {
> > +    if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_NETWORK_SERVER)) {
> > +        error_setg(errp, "chardev \"%s\" does not provide a network
> > server",
> > +                   opts->chardev);
> >          return NULL;
> >      }
> >
> 
> It doesn't have to be a server, it can be a client too. Too bad we don't
> check that aspect in vhost-user-test already.

Oh, i see. Will change this then.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|