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

[Nikolay Nikolaev]

On Thu, Jun 5, 2014 at 7:08 PM, Eric Blake <address@hidden> wrote:

On 05/27/2014 06:06 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.
>
> Signed-off-by: Antonios Motakis <address@hidden>
> Signed-off-by: Nikolay Nikolaev <address@hidden>
> ---
>  hmp-commands.hx    |    4 +-
>  hw/net/vhost_net.c |    4 ++
>  net/hub.c          |    1
>  net/net.c          |   25 ++++++-----
>  net/vhost-user.c   |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qapi-schema.json   |   19 ++++++++-
>  qemu-options.hx    |   18 ++++++++
>  7 files changed, 169 insertions(+), 16 deletions(-)
>


>

> +static int net_vhost_chardev_opts(const char *name, const char *value,
> +        void *opaque)

Indentation is off (second line should be aligned to the ( of the first).

OK, will fix it.

> +{
> +    VhostUserChardevProps *props = opaque;
> +
> +    if (strcmp(name, "backend") == 0 && strcmp(value, "socket") == 0) {
> +        props->is_socket = 1;

s/1/true/ - when a field is a boolean, assign it boolean constants.

OK, will fix all of them.

> +    } else if (strcmp(name, "path") == 0) {
> +        props->is_unix = 1;
> +    } else if (strcmp(name, "server") == 0) {
> +        props->is_server = 1;
> +    } else {
> +        error_report("vhost-user does not support a chardev"
> +                     " with the following option:\n %s = %s",
> +                     name, value);
> +        props->has_unsupported = 1;

and 3 more times.

> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +static CharDriverState *net_vhost_parse_chardev(
> +        const NetdevVhostUserOptions *opts)

Unusual indentation, but I see your dilemma of fitting 80 columns.

What woudl be the right approach here, is leaving it 84 colums acceptable:

static CharDriverState *net_vhost_parse_chardev(const NetdevVhostUserOptions *opts)

Or split it like this:
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\n", opts->chardev);
> +        return 0;

s/0/NULL/ - much nicer to use the named constant when referring to a
null pointer.

OK - fixed on all places.
 

> +    }
> +
> +    /* 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\n",
> +                     opts->chardev);
> +        return 0;
> +    }
> +
> +    if (props.has_unsupported) {
> +        error_report("chardev \"%s\" has an unsupported option\n",
> +                opts->chardev);
> +        return 0;

2 more instances

> +    }
> +
> +    qemu_chr_fe_claim_no_fail(chr);
> +
> +    return chr;
> +}
> +
> +static int net_vhost_check_net(QemuOpts *opts, void *opaque)
> +{
> +    const char *name = opaque;
> +    const char *driver, *netdev;
> +    const char virtio_name[] = "virtio-net-";
> +
> +    driver = qemu_opt_get(opts, "driver");
> +    netdev = qemu_opt_get(opts, "netdev");
> +
> +    if (!driver || !netdev) {
> +        return 0;
> +    }
> +
> +    if ((strcmp(netdev, name) == 0)
> +            && (strncmp(driver, virtio_name, strlen(virtio_name)) != 0)) {

Unusual indentation and spurious ():

if (strcmp(netdev, name) == 0 &&
    strncmp(driver, virtio_name, strlen(virtio_name)) != 0) {

OK - thanks for the providing a solution.

> +        error_report("vhost-user requires frontend driver virtio-net-*");

How many of these error_report() should be using Error **errp logistics
instead?

I don't see this 'errp' logic applied here. These are static functions called in the init function to check for compatible command line parameters.

This init fucntion is part of 'net_client_init_fun[]" which does not provide 'Error **errp' argument. The way I see it, there is no way to propagate the 'errp' up. Or am I getting this wrong?

 

> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  int net_init_vhost_user(const NetClientOptions *opts, const char *name,
>                     NetClientState *peer)

Pre-existing indentation problem, but you could fix it here (or if you
introduced it earlier in the series, fix it there)

Will fix it.

>  {
> -    return net_vhost_user_init(peer, "vhost_user", 0, 0, 0);
> +    const NetdevVhostUserOptions *vhost_user_opts;
> +    CharDriverState *chr;
> +    bool vhostforce;
> +
> +    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> +    vhost_user_opts = opts->vhost_user;
> +
> +    chr = net_vhost_parse_chardev(vhost_user_opts);
> +    if (!chr) {
> +        error_report("No suitable chardev found\n");

No \n in error_report() messages.

OK.

> +        return -1;
> +    }
> +
> +    /* verify net frontend */
> +    if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
> +                          (void *)name, true) == -1) {

This is C; why do you need a cast to (void*)?

Because of the constness in 'const char *name'. I am casting to 'char * now', is it better?

> +        return -1;
> +    }
> +
> +    /* vhostforce for non-MSIX */
> +    if (vhost_user_opts->has_vhostforce) {
> +        vhostforce = vhost_user_opts->vhostforce;
> +    } else {
> +        vhostforce = false;
> +    }
> +
> +    return net_vhost_user_init(peer, "vhost_user", name, chr, vhostforce);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 1f28177..f458dd8 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3264,6 +3264,22 @@
>      '*devname':    'str' } }
>
>  ##
> +# @NetdevVhostUserOptions
> +#
> +# Vhost-user network backend
> +#
> +# @path: control socket path
> +#
> +# @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> +#
> +# Since 2.1
> +##
> +{ 'type': 'NetdevVhostUserOptions',
> +  'data': {
> +    'chardev':        'str',
> +    '*vhostforce':    'bool' } }

Bikeshedding - is 'vhost-force' any more legible?

'vhost-force' it is.

--
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Thanks!

regards,

Nikolay Nikolaev