| Message ID | 1475833793-8542-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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) {
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(-)