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 <ebl...@redhat.com> wrote:
> On 2/4/21 10:25 AM, phillip.en...@gmail.com wrote:
> > From: Phillip Tennen <phil...@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 <phil...@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
>
>
