Re: [PATCH v2] net/macos: implement vmnet-based network device

[Markus Armbruster]

Phillip Tennen <phillip.ennen@gmail.com> writes:

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

We *try* not to be jerks ;)

Your reply looks fine to me.

> Phillip
>
> On Tue, Mar 2, 2021 at 11:49 AM Markus Armbruster <armbru@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.ennen@gmail.com writes:
>>
>> > 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.
>> >
>>
> [...]
>
>> > 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.

Clarity is in the eye of the beholder.  We try to find the sweet spot
between bafflingly terse and tiresomely verbose.


>> > +#                      (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.

In bridged mode, none.

In host or shared mode, either all three or none.

Correct?

> [...]
>
>> > +#                      (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.

Yes, except you don't need a @mode member, you can derive the mode from
Netdev member @type.

Clear now?