Re: [Qemu-devel] [PATCH v10-fix 15/18] Add the vhost-user netdev backend to the command line

[Michael S. Tsirkin]

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
> > address@hidden
> 
> [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
>