From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48467) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WuLiB-0002Lr-4P for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:03:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WuLi5-0000Z1-7I for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:03:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51850) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WuLi4-0000Yn-US for qemu-devel@nongnu.org; Tue, 10 Jun 2014 09:03:29 -0400 Date: Tue, 10 Jun 2014 16:03:53 +0300 From: "Michael S. Tsirkin" Message-ID: <20140610130353.GB27702@redhat.com> References: <20140610100157.11064.61717.stgit@3820> <5396F5E4.6010900@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5396F5E4.6010900@redhat.com> Subject: Re: [Qemu-devel] [PATCH v10-fix 15/18] Add the vhost-user netdev backend to the command line List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: snabb-devel@googlegroups.com, qemu-devel@nongnu.org, Nikolay Nikolaev , luke@snabb.co, a.motakis@virtualopensystems.com, tech@virtualopensystems.com On Tue, Jun 10, 2014 at 06:11:16AM -0600, Eric Blake wrote: > On 06/10/2014 04:02 AM, Nikolay Nikolaev wrote: > > The supplied chardev id will be inspected for supported options. Only > > a socket backend, with a set path (i.e. a Unix socket) and optionally > > the server parameter set, will be allowed. Other options (nowait, telnet) > > will make the chardev unusable and the netdev will not be initialised. > > > > Additional checks for validity: > > - requires `-numa node,memdev=..` > > - requires `-device virtio-net-*` > > > > The `vhostforce` option is used to force vhost-net when we deal with > > non-MSIX guests. > > Here you call it vhostforce...[1] > > > > > +static int net_vhost_chardev_opts(const char *name, const char *value, > > + void *opaque) > > +{ > > + 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) { > > + props->is_server = true; > > + } else { > > + error_report("vhost-user does not support a chardev" > > + " with the following option:\n %s = %s", > > + name, value); > > + props->has_unsupported = true; > > + return -1; > > Here you reported an error...[2] > > > + } > > + return 0; > > +} > > + > > +static CharDriverState *net_vhost_parse_chardev(const NetdevVhostUserOptions *opts) > > +{ > > + CharDriverState *chr = qemu_chr_find(opts->chardev); > > + VhostUserChardevProps props; > > + > > + if (chr == NULL) { > > + error_report("chardev \"%s\" not found", opts->chardev); > > + return NULL; > > + } > > + > > + /* inspect chardev opts */ > > + memset(&props, 0, sizeof(props)); > > + qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props, false); > > + > > + if (!props.is_socket || !props.is_unix) { > > + error_report("chardev \"%s\" is not a unix socket", > > + opts->chardev); > > + return NULL; > > + } > > + > > + if (props.has_unsupported) { > > + error_report("chardev \"%s\" has an unsupported option", > > + opts->chardev); > > + return NULL; > > [2]...and again another error. One report is sufficient. For that > matter, I highly doubt you even need a has_unsupported member - since > you always error out early before allowing the device to be created, it > is just redundant information and will always be false for any device > that gets past creation without reporting an error. > > > > +## > > +{ 'type': 'NetdevVhostUserOptions', > > + 'data': { > > + 'chardev': 'str', > > + '*vhost-force': 'bool' } } > > [1]...and here you call it vhost-force... > Should be vhostforce, consistent with tap. > > +Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should > > +be a unix domain socket backed one. The vhost-user uses a specifically defined > > +protocol to pass vhost ioctl replacement messages to an application on the other > > +end of the socket. On non-MSIX guests, the feature can be forced with > > +@var{vhostforce}. > > [1]...yet document it as vhostforce. > > If this has already been applied, then you need to submit a followup patch. Pls do, use fixup! "original subject" for clarity. > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >