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

[Phillip Tennen <phillip.ennen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7994 bytes --]

Thanks very much for taking a look!

As per my understanding of the submission process, I will resubmit this
patchset (sans my self-introduction =) )
in a new [PATCH v2] thread, incorporating the changes you pointed out here.

> 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.

I'm unfamiliar with the QAPI codegen and what it supports~
The `mode` param is shared by all three mode types. Thus,
could I make the `NetdevVmnetOptions` contain a mode field
and a mode-specific union of options, to avoid the direct union-union
nesting?

Phillip

On Thu, Feb 4, 2021 at 8:51 PM Eric Blake <eblake@redhat.com> wrote:

> 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
>
>

[-- Attachment #2: Type: text/html, Size: 10677 bytes --]