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

[Phillip Tennen]

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

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