On Thu, Jun 5, 2014 at 7:08 PM, Eric Blake <ebl...@redhat.com> 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 <a.mota...@virtualopensystems.com>
> > Signed-off-by: Nikolay Nikolaev <n.nikol...@virtualopensystems.com>
> > ---
> > 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
