Re: [PATCH 2/7] net/vmnet: add new netdevs to qapi/net

[Eric Blake <eblake@redhat.com>]

On Thu, Jun 17, 2021 at 05:32:41PM +0300, Vladislav Yaroshchuk wrote:
> Created separate netdev per each vmnet operating mode
> because they use quite different settings. Especially since
> macOS 11.0 (vmnet.framework API gets lots of updates)
> 
> Three new netdevs are added:
> - vmnet-host
> - vmnet-shared
> - vmnet-bridged
> 
> Signed-off-by: Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>
> ---

> +++ b/qapi/net.json
> @@ -452,6 +452,89 @@
>      '*vhostdev':     'str',
>      '*queues':       'int' } }
>  
> +##
> +# @NetdevVmnetHostOptions:
> +#
> +# vmnet (host mode) network backend.
> +#
> +# Allows the vmnet interface to communicate with
> +# other vmnet interfaces that are in host mode and also with the native host.
> +#
> +# @dhcpstart: The starting IPv4 address to use for the interface. Must be in the
> +#             private IP range (RFC 1918). Must be specified along
> +#             with @dhcpend and @subnetmask.
> +#             This address is used as the gateway address. The subsequent address
> +#             up to and including dhcpend are  placed in the DHCP pool.
> +#
> +# @dhcpend: The DHCP IPv4 range end address to use for the interface. Must be in
> +#           the private IP range (RFC 1918). Must be specified along
> +#           with @dhcpstart and @subnetmask.
> +#
> +# @subnetmask: The IPv4 subnet mask to use on the interface. Must be specified
> +#              along with @dhcpstart and @subnetmask.
> +#
> +#
> +# Since: 6.1,
> +##

Same comments about 6.1 vs. 6.2 as before (I'll quit pointing it out).
Spurious trailing comma.

> +{ 'struct': 'NetdevVmnetHostOptions',
> +  'data': {
> +    '*dhcpstart':   'str',
> +    '*dhcpend':     'str',
> +    '*subnetmask':  'str'

Hmm. You listed all three as optional, but document that they must all
be specified together.  Why not just make them all required, and
simplify the documentation?

> +  },
> +  'if': 'defined(CONFIG_VMNET)' }
> +
> +##
> +# @NetdevVmnetSharedOptions:
> +#
> +# vmnet (shared mode) network backend.
> +#
> +# Allows traffic originating from the vmnet interface to reach the
> +# Internet through a network address translator (NAT). The vmnet interface
> +# can also communicate with the native host. By default, the vmnet interface
> +# is able to communicate with other shared mode interfaces. If a subnet range
> +# is specified, the vmnet interface can communicate with other shared mode
> +# interfaces on the same subnet.
> +#
> +# @dhcpstart: The starting IPv4 address to use for the interface. Must be in the
> +#             private IP range (RFC 1918). Must be specified along
> +#             with @dhcpend and @subnetmask.
> +#             This address is used as the gateway address. The subsequent address
> +#             up to and including dhcpend are  placed in the DHCP pool.

Spurious double space.

> +#
> +# @dhcpend: The DHCP IPv4 range end address to use for the interface. Must be in
> +#           the private IP range (RFC 1918). Must be specified along
> +#           with @dhcpstart and @subnetmask.
> +#
> +# @subnetmask: The IPv4 subnet mask to use on the interface. Must be specified
> +#              along with @dhcpstart and @subnetmask.
> +#
> +#
> +# Since: 6.1,
> +##
> +{ 'struct': 'NetdevVmnetSharedOptions',
> +  'data': {
> +    '*dhcpstart':    'str',
> +    '*dhcpend':      'str',
> +    '*subnetmask':   'str'
> +  },
> +  'if': 'defined(CONFIG_VMNET)' }
> +
> +##
> +# @NetdevVmnetBridgedOptions:
> +#
> +# vmnet (bridged mode) network backend.
> +#
> +# Bridges the vmnet interface with a physical network interface.
> +#
> +# @ifname: The name of the physical interface to be bridged.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'NetdevVmnetBridgedOptions',
> +  'data': { 'ifname': 'str' },
> +  'if': 'defined(CONFIG_VMNET)' }
> +
>  ##
>  # @NetClientDriver:
>  #
> @@ -460,11 +543,16 @@
>  # Since: 2.7
>  #
>  #        @vhost-vdpa since 5.1
> -#        @vmnet since 6.1

Why are you dropping vmnet?  Especially since you just added it in the
previous patch?  That feels like needless churn.

> +#        @vmnet-host since 6.1
> +#        @vmnet-shared since 6.1
> +#        @vmnet-bridged since 6.1
>  ##
>  { 'enum': 'NetClientDriver',
>    'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> -            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa', 'vmnet' ] }
> +            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa',
> +            { 'name': 'vmnet-host', 'if': 'defined(CONFIG_VMNET)' },
> +            { 'name': 'vmnet-shared', 'if': 'defined(CONFIG_VMNET)' },
> +            { 'name': 'vmnet-bridged', 'if': 'defined(CONFIG_VMNET)' }] }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org