Re: [PATCH 2/2] net: implement vmnet-based netdev

[Eric Blake <eblake@redhat.com>]

On 2/4/21 10:25 AM, phillip.ennen@gmail.com wrote:
> From: Phillip Tennen <phillip@axleos.com>
> 
> This patch implements a new netdev device, reachable via -netdev
> vmnet-macos, that’s backed by macOS’s vmnet framework.
> 
> The vmnet framework provides native bridging support, and its usage in
> this patch is intended as a replacement for attempts to use a tap device
> via the tuntaposx kernel extension. Notably, the tap/tuntaposx approach
> never would have worked in the first place, as QEMU interacts with the
> tap device via poll(), and macOS does not support polling device files.
> 

> This is my first QEMU contribution, so please feel free to let me know
> what I’ve missed or what needs improving. Thanks very much for taking a
> look =)

This paragraph would fit better...

> 
> Signed-off-by: Phillip Tennen <phillip@axleos.com>
> ---

...here, after the --- marker.  It's useful to the reviewer (and welcome
to the community, by the way!), but does not need to be enshrined in git
history.

>  configure         |   2 +-
>  net/clients.h     |   6 +
>  net/meson.build   |   1 +
>  net/net.c         |   3 +
>  net/vmnet-macos.c | 444 ++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/net.json     |  64 ++++++-
>  qemu-options.hx   |   9 +

I'm focusing mainly on the UI aspects, and did not look closely at the rest.

> +++ b/qapi/net.json
> @@ -450,6 +450,65 @@
>      '*vhostdev':     'str',
>      '*queues':       'int' } }
>  
> +##
> +# @VmnetOperatingMode:
> +#
> +# An enumeration of the I/O operation types
> +#
> +# @host: the guest may communicate with the host 
> +#        and other guest network interfaces
> +#
> +# @shared: the guest may reach the Internet through a NAT, 
> +#          and may communicate with the host and other guest 
> +#          network interfaces
> +#
> +# @bridged: the guest's traffic is bridged with a 
> +#           physical network interface of the host
> +#
> +# Since: 5.3

The next release will be 6.0, not 5.3.

> +##
> +{ 'enum': 'VmnetOperatingMode',
> +  'data': [ 'host', 'shared', 'bridged' ] }
> +
> +##
> +# @NetdevVmnetOptions:
> +#
> +# vmnet network backend (only available on macOS)
> +#
> +# @mode: the operating mode vmnet should run in
> +#
> +# @ifname: the physical network interface to bridge with 
> +#          (only valid with mode=bridged)

If this is only valid with bridged, then you should use a union type,
where only the bridged branch supports this member.  That is more
typesafe than always supporting it in the schema and having to
semantically filter it out after the fact.

> +#
> +# @dhcp_start_address: the gateway address to use for the interface. 

New QAPI additions should favor names with '-' rather than '_', as in
dhcp-start-address; the exception is if you are closely copying another
older interface that already used _.

> +#                      The range to dhcp_end_address is placed in the DHCP pool.
> +#                      (only valid with mode=host|shared)
> +#                      (must be specified with dhcp_end_address and 
> +#                       dhcp_subnet_mask)
> +#                      (allocated automatically if unset)
> +#
> +# @dhcp_end_address: the DHCP IPv4 range end address to use for the interface. 
> +#                      (only valid with mode=host|shared)
> +#                      (must be specified with dhcp_start_address and 
> +#                       dhcp_subnet_mask)
> +#                      (allocated automatically if unset)
> +#
> +# @dhcp_subnet_mask: the IPv4 subnet mask (string) to use on the interface.
> +#                    (only valid with mode=host|shared)
> +#                    (must be specified with dhcp_start_address and 
> +#                     dhcp_end_address)
> +#                    (allocated automatically if unset)
> +#
> +# Since: 5.3

6.0

> +##
> +{ 'struct': 'NetdevVmnetOptions',
> +  'data': {
> +    'mode':        'VmnetOperatingMode',
> +    '*ifname':        'str',
> +    '*dhcp_start_address':      'str',
> +    '*dhcp_end_address':        'str',
> +    '*dhcp_subnet_mask':        'str' } }

In addition to my suggestion of making this a union rather than a
struct, you probably also want to include an 'if' tag so that the struct
is present only on MacOS builds (it's nicer for introspection to not
advertise something that won't work).

> +
>  ##
>  # @NetClientDriver:
>  #
> @@ -461,7 +520,7 @@
>  ##
>  { 'enum': 'NetClientDriver',
>    'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> -            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa' ] }
> +            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa', 'vmnet-macos' ] }

Missing a doc mention that 'vmnet-macos' is new to 6.0.

>  
>  ##
>  # @Netdev:
> @@ -490,7 +549,8 @@
>      'hubport':  'NetdevHubPortOptions',
>      'netmap':   'NetdevNetmapOptions',
>      'vhost-user': 'NetdevVhostUserOptions',
> -    'vhost-vdpa': 'NetdevVhostVDPAOptions' } }
> +    'vhost-vdpa': 'NetdevVhostVDPAOptions',
> +    'vmnet-macos': 'NetdevVmnetOptions' } }

Adding Markus in cc; right now, I don't think QAPI supports a union type
as a branch to another union type. There's nothing that technically
would prevent us from doing that, just the grunt work of modifying the
QAPI code generator to support it.

>  
>  ##
>  # @NetFilterDirection:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9172d51659..ec6b40b079 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2483,6 +2483,15 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>  #ifdef __linux__
>      "-netdev vhost-vdpa,id=str,vhostdev=/path/to/dev\n"
>      "                configure a vhost-vdpa network,Establish a vhost-vdpa netdev\n"
> +#endif
> +#ifdef CONFIG_DARWIN
> +    "-netdev vmnet-macos,id=str,mode=bridged[,ifname=ifname]\n"
> +    "         configure a macOS-provided vmnet network in \"physical interface bridge\" mode\n"
> +    "         the physical interface to bridge with defaults to en0 if unspecified\n"
> +    "-netdev vmnet-macos,id=str,mode=host|shared\n"
> +    "                     [,dhcp_start_address=addr,dhcp_end_address=addr,dhcp_subnet_mask=mask]\n"
> +    "         configure a macOS-provided vmnet network in \"host\" or \"shared\" mode\n"
> +    "         the DHCP configuration will be set automatically if unspecified\n"
>  #endif
>      "-netdev hubport,id=str,hubid=n[,netdev=nd]\n"
>      "                configure a hub port on the hub with ID 'n'\n", QEMU_ARCH_ALL)
> 

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