Re: [Qemu-devel] [PATCH 6/6] Add new vhost-user netdev backend

[Antonios Motakis]

On Tue, Dec 10, 2013 at 11:29 PM, Eric Blake <address@hidden> wrote:

On 12/09/2013 05:25 PM, Antonios Motakis wrote:
> Add a new QEMU netdev backend that is intended to invoke vhost_net
> with the vhost-user backend. Also decouple virtio-net from the tap
> backend.
>
> Signed-off-by: Antonios Motakis <address@hidden>
> Signed-off-by: Nikolay Nikolaev <address@hidden>
> ---

> +++ b/include/net/vhost-user.h
> @@ -0,0 +1,17 @@
> +/*
> + * vhost-user.h
> + *
> + * Copyright (c) 2013 Virtual Open Systems Sarl.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.

Can you please use GPLv2+ (that is, add the "or later" clause)?  Yes, we
already have GPLv2-only files, but I'd like to avoid adding even more of
them.

> +++ b/net/vhost-user.c
> @@ -0,0 +1,95 @@
> +/*
> + * vhost-user.c
> + *
> + * Copyright (c) 2013 Virtual Open Systems Sarl.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.

Same question applies to all new files added throughout this series.

> +++ b/qapi-schema.json
> @@ -3009,11 +3009,24 @@
>      'hubid':     'int32' } }
>
>  ##
> +# @NetdevVhostUserOptions
> +#
> +# Vhost-user network backend
> +#
> +# @file: control socket path

What does it mean when 'file' is not present?  Is there a default value?
 Normally, we mark '#optional' in the docs for an optional argument.

> +#
> +# Since 2.0
> +##
> +{ 'type': 'NetdevVhostUserOptions',
> +  'data': {
> +    '*file': 'str' } }

Or is file always present, in which case this should be 'file' instead
of '*file'?

File should always be present, so we will change it to file without the asterisk.

 

> +
> +##
>  # @NetClientOptions
>  #
>  # A discriminated record of network device traits.
>  #
> -# Since 1.2
> +# Since 2.0

Wrong.  NetClientOptions has existed since 1.2; but some of the branches
of the union are newer.  The way we have documented that elsewhere looks
more like:

# A discriminated record of network device traits.

# @vde: traits for VDE
# @dump: traits when using the device to dump all traffic
# @bridge: traits for a bridge device
# @hubport: traits for a hub port
# @vhost-user: traits for a vhost-user (since 2.0)
#
# Since 1.2

Thanks for your feedback, we will take it into account for the next version of the series.

Antonios

>  ##
>  { 'union': 'NetClientOptions',
>    'data': {
> @@ -3025,7 +3038,8 @@
>      'vde':      'NetdevVdeOptions',
>      'dump':     'NetdevDumpOptions',
>      'bridge':   'NetdevBridgeOptions',
> -    'hubport':  'NetdevHubPortOptions' } }
> +    'hubport':  'NetdevHubPortOptions',
> +    'vhost-user': 'NetdevVhostUserOptions' } }
>
>  ##
>  # @NetLegacy
>

--

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