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
>
