Markus, thanks for the review. I apologize for my lateness in getting back
to you.
I've integrated most of your suggestions, and will submit a v5 that
incorporates them. I've left a couple comments and questions for you below.
Aside: I haven't responded inline to emails like this before, I'm hoping it
shows
up correctly for you! I appreciate how understanding everyone's been
towards my
newness to this development & review format. I cut out the irrelevant bits
for brevity and am unsure if that breaks anything.
Phillip
On Tue, Mar 2, 2021 at 11:49 AM Markus Armbruster <arm...@redhat.com> wrote:
> Phillip, this doesn't apply anymore. I'm reviewing the QAPI schema part
> anyway.
>
> Peter, there is a question for you below. Search for "Sphinx".
>
> phillip.en...@gmail.com writes:
>
> > 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.
> >
>
[...]
> > diff --git a/qapi/net.json b/qapi/net.json
> > index c31748c87f..e4d4143243 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -450,6 +450,115 @@
> > '*vhostdev': 'str',
> > '*queues': 'int' } }
> >
> > +##
> > +# @VmnetOperatingMode:
> > +#
> > +# The operating modes in which a vmnet netdev can run
> > +# Only available on macOS
>
> Please end these sentences with a period.
>
> I'm not sure we need "Only available on macOS". Rendered documentation
> shows the 'if' like
>
> [...]
> > +# (only valid with mode=host|shared)
>
> Isn't that trivial? The type's only use is as union branch for modes
> host and shared.
>
True. I added comments like this for clarity, but I accept that the schema
should make it clear alone.
>
> > +# (must be specified with dhcp-end-address and
> > +# dhcp-subnet-mask)
>
> Does that mean you have to specify all three parameters or none?
>
That's correct. You may provide either none or all three.
[...]
> > +# (allocated automatically if unset)
>
> How?
>
vmnet automatically allocates specifics like the MAC address, DHCP pool,
etc,
if not explicitly supplied. I'll add some wording to this effect.
[...]
>
> > +#
> > +# Since: 6.0
> > +##
> > +{ 'struct': 'NetdevVmnetOptions',
> > + 'data': {'options': 'NetdevVmnetModeOptions' },
> > + 'if': 'defined(CONFIG_DARWIN)' }
> > +
>
> Awkward.
>
> You can't use make NetdevVmnetModeOptions a branch of union Netdev,
> because NetdevVmnetModeOptions is a union, and a branch must be a
> struct. To work around, you wrap struct NetdevVmnetOptions around union
> NetdevVmnetModeOptions.
>
> NetdevVmnetModeOptions has no common members other than the union
> discriminator. Why not add them as three branches to Netdev?
>
> Just to be sure I understand, you're proposing adding 3 new fields to
Netdev,
like so:
'vmnet-macos-bridged': { 'type': 'NetdevVmnetModeOptionsBridged',
'if': 'defined(CONFIG_DARWIN)' },
'vmnet-macos-host': { 'type': 'NetdevVmnetModeOptionsHostOrShared',
'if': 'defined(CONFIG_DARWIN)' },
'vmnet-macos-shared': { 'type': 'NetdevVmnetModeOptionsHostOrShared',
'if': 'defined(CONFIG_DARWIN)' },
... where each of those "ModeOptions" structs contains a new "mode" field
extracted from the union. Did I get your intent right? I'm assuming there
wouldn't be issues with "vmnet-macos" referenced elsewhere.
Thank you!
Phillip
