From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48023) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsSJr-0007Fd-Uj for qemu-devel@nongnu.org; Fri, 07 Oct 2016 06:24:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsSJn-0002Lp-VT for qemu-devel@nongnu.org; Fri, 07 Oct 2016 06:23:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49934) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsSJn-0002L6-Jd for qemu-devel@nongnu.org; Fri, 07 Oct 2016 06:23:55 -0400 Date: Fri, 7 Oct 2016 11:23:51 +0100 From: "Daniel P. Berrange" Message-ID: <20161007102350.GI26332@redhat.com> Reply-To: "Daniel P. Berrange" References: <1475833793-8542-1-git-send-email-berrange@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] vhost-user: don't poke at chardev internal QemuOpts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: qemu-devel@nongnu.org, Michal Privoznik , Markus Armbruster , Paolo Bonzini , "Michael S. Tsirkin" On Fri, Oct 07, 2016 at 10:12:23AM +0000, Marc-Andr=C3=A9 Lureau wrote: > Hi >=20 > On Fri, Oct 7, 2016 at 1:50 PM Daniel P. Berrange > wrote: >=20 > > 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=3D0x0, func=3D0x55b= af696b650 > > , opaque=3D0x7ffc51368c00, errp=3D0x7ffc51368= e48) 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=3D0x0, func=3D0x55b= af696b650 > > , opaque=3D0x7ffc51368c00, errp=3D0x7ffc51368= e48) at > > util/qemu-option.c:617 > > #1 0x000055baf696b7da in net_vhost_parse_chardev (opts=3D0x55baf8ff= 9260, > > errp=3D0x7ffc51368e48) at net/vhost-user.c:314 > > #2 0x000055baf696b985 in net_init_vhost_user (netdev=3D0x55baf8ff92= 50, > > name=3D0x55baf879d270 "hostnet2", peer=3D0x0, errp=3D0x7ffc51368e48) = at > > net/vhost-user.c:360 > > #3 0x000055baf6960216 in net_client_init1 (object=3D0x55baf8ff9250, > > is_netdev=3Dtrue, errp=3D0x7ffc51368e48) at net/net.c:1051 > > #4 0x000055baf6960518 in net_client_init (opts=3D0x55baf776e7e0, > > is_netdev=3Dtrue, errp=3D0x7ffc51368f00) at net/net.c:1108 > > #5 0x000055baf696083f in netdev_add (opts=3D0x55baf776e7e0, > > errp=3D0x7ffc51368f00) at net/net.c:1186 > > #6 0x000055baf69608c7 in qmp_netdev_add (qdict=3D0x55baf7afaf60, > > ret=3D0x7ffc51368f50, errp=3D0x7ffc51368f48) at net/net.c:1205 > > #7 0x000055baf6622135 in handle_qmp_command (parser=3D0x55baf77fb59= 0, > > tokens=3D0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978 > > #8 0x000055baf6a9d099 in json_message_process_token > > (lexer=3D0x55baf77fb598, input=3D0x55baf75acd20, type=3DJSON_RCURLY, = x=3D113, y=3D19) > > at qobject/json-streamer.c:105 > > #9 0x000055baf6abf7aa in json_lexer_feed_char (lexer=3D0x55baf77fb5= 98, > > ch=3D125 '}', flush=3Dfalse) at qobject/json-lexer.c:319 > > #10 0x000055baf6abf8f2 in json_lexer_feed (lexer=3D0x55baf77fb598, > > buffer=3D0x7ffc51369170 "}R\204\367\272U", size=3D1) at qobject/json-= lexer.c:369 > > #11 0x000055baf6a9d13c in json_message_parser_feed > > (parser=3D0x55baf77fb590, buffer=3D0x7ffc51369170 "}R\204\367\272U", = size=3D1) at > > qobject/json-streamer.c:124 > > #12 0x000055baf66221f7 in monitor_qmp_read (opaque=3D0x55baf77fb530, > > buf=3D0x7ffc51369170 "}R\204\367\272U", size=3D1) at > > /path/to/qemu.git/monitor.c:3994 > > #13 0x000055baf6757014 in qemu_chr_be_write_impl (s=3D0x55baf7610a40= , > > buf=3D0x7ffc51369170 "}R\204\367\272U", len=3D1) at qemu-char.c:387 > > #14 0x000055baf6757076 in qemu_chr_be_write (s=3D0x55baf7610a40, > > buf=3D0x7ffc51369170 "}R\204\367\272U", len=3D1) at qemu-char.c:399 > > #15 0x000055baf675b3b0 in tcp_chr_read (chan=3D0x55baf90244b0, > > cond=3DG_IO_IN, opaque=3D0x55baf7610a40) at qemu-char.c:2927 > > #16 0x000055baf6a5d655 in qio_channel_fd_source_dispatch > > (source=3D0x55baf7610df0, callback=3D0x55baf675b25a , > > user_data=3D0x55baf7610a40) 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=3D12600000= 0) at > > main-loop.c:258 > > #20 0x000055baf69d38ad in main_loop_wait (nonblocking=3D0) at > > main-loop.c:506 > > #21 0x000055baf676587b in main_loop () at vl.c:1908 > > #22 0x000055baf676d3bf in main (argc=3D101, argv=3D0x7ffc5136a6c8, > > envp=3D0x7ffc5136a9f8) at vl.c:4604 > > (gdb) p opts > > $1 =3D (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 > > --- > > > > 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 *filen= ame); > > > > 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 =3D 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 *valu= e, > > - Error **errp) > > -{ > > - VhostUserChardevProps *props =3D opaque; > > - > > - if (strcmp(name, "backend") =3D=3D 0 && strcmp(value, "socket") = =3D=3D 0) { > > - props->is_socket =3D true; > > - } else if (strcmp(name, "path") =3D=3D 0) { > > - props->is_unix =3D true; > > - } else if (strcmp(name, "server") =3D=3D 0) { > > - } else { > > - error_setg(errp, > > - "vhost-user does not support a chardev with optio= n > > %s=3D%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 =3D qemu_chr_find(opts->chardev); > > - VhostUserChardevProps props; > > > > if (chr =3D=3D 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; > > } > > >=20 > 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 --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr= / :|