Re: [PATCH v4 2/6] net/vmnet: add vmnet backends to qapi/net

[Markus Armbruster]

I apologize for taking so long to respond.  I spotted v5, but haven't
reviewed it.  Please read my response, then assess whether you need v6.
If not, let me know, so I can have a look at v5.

Vladislav Yaroshchuk <yaroshchuk2000@gmail.com> writes:

> --000000000000325dde05d084e6e4
> Content-Type: text/plain; charset="UTF-8"
> Content-Transfer-Encoding: quoted-printable
>
> =D0=B2=D1=82, 9 =D0=BD=D0=BE=D1=8F=D0=B1. 2021 =D0=B3. =D0=B2 08:41, Markus=
>  Armbruster <armbru@redhat.com>:
>
>> Vladislav Yaroshchuk <yaroshchuk2000@gmail.com> writes:
>>
>> > Create separate netdevs for each vmnet operating mode:
>> > - vmnet-host
>> > - vmnet-shared
>> > - vmnet-bridged
>> >
>> > Signed-off-by: Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>
>>
>> [...]
>>
>> > diff --git a/qapi/net.json b/qapi/net.json
>> > index 7fab2e7cd8..087cdf0546 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -452,6 +452,112 @@
>> >      '*vhostdev':     'str',
>> >      '*queues':       'int' } }
>> >
>> > +##
>> > +# @NetdevVmnetHostOptions:
>> > +#
>> > +# vmnet (host mode) network backend.
>> > +#
>> > +# Allows the vmnet interface to communicate with other vmnet
>> > +# interfaces that are in host mode and also with the native host.
>> > +#
>> > +# @dhcpstart: The starting IPv4 address to use for the interface.
>> > +#             Must be in the private IP range (RFC 1918). Must be
>> > +#             specified along with @dhcpend and @subnetmask.
>> > +#             This address is used as the gateway address. The
>> > +#             subsequent address up to and including dhcpend are
>> > +#             placed in the DHCP pool.
>>
>> In QMP, we separate words by dashes, like @dhcp-start.  We may prefer
>> not to here, for consistency with NetdevUserOptions, ...
>>
>> +#
>> > +# @dhcpend: The DHCP IPv4 range end address to use for the
>> > +#           interface. Must be in the private IP range (RFC 1918).
>> > +#           Must be specified along with @dhcpstart and @subnetmask.
>>
>> ... and here, to match @dhcpstart.
>>
>> Yes, I did not use dashed exactly because of this: to be consistent with 
>> NetdevUserOptions.
>
>> +#
>> > +# @subnetmask: The IPv4 subnet mask to use on the interface. Must
>> > +#              be specified along with @dhcpstart and @subnetmask.
>>
>> @subnet-mask, please.
>>
>
> So I think we can leave `dhcpstart`, `dhcpend`, but add a dash to
> `subnet-mask`.

Yes, please.

>> No support for IPv6?
>>
>>
> It's supported. vmnet-shared tested with link-local addresses.
> Also some configurable options exist, but only for 'shared' mode.
> I can try to add them in the next patch series version.

Or a later patch.

>> +#
>> > +# @isolated: Enable isolation for this interface. Interface isolation
>> > +#            ensures that network communication between multiple
>> > +#            vmnet interfaces instances is not possible.
>> > +#
>> > +# @net-uuid: The identifier (UUID) to uniquely identify the network.
>> > +#            If provided, no DHCP service is provided on this network
>> > +#            and the interface is added to an isolated network with
>> > +#            the specified identifier.
>>
>> Out of curiosity: identify to whom?
>>
>> If I set @net-uuid, I get an isolated network with a UUID, and if I set
>> "isolated": true, I get one without a UUID.  When would I want the
>> former, and when the latter?
>>
>>
> `if I set "isolated": true, I get one without a UUID.` is an
> incorrect statement.
>
> vmnet.framework isolation features are designed a bit weirdly (just my
> opinion).
> Most things happen on the macOS side, and we just can provide some
> configuration
> options to vmnet.framework as the user provides options to QEMU.
>
> Firstly, if you set @net-uuid, you're putting the interface into an
> isolated network.
> You can set the same @net-uuid on multiple vmnet-host interfaces, also
> outside
> QEMU, it can be any vmnet interface started in 'host' mode, ex. xhyve's
> netdev
> if they support this. All of them can communicate with each other, but only
> inside
> this net identified by the @net-uuid. They can't communicate with
> interfaces from
> network with id !=3D ${our set @net-uuid}.
>
> "@isolated" is a more prohibitive mode. "Isolated" interface is not able to
> communicate
> with anyone except the host.
>
> What happens when I set "isolated": false together with @net-uuid?
>
>
> Let's compare (vmnet-host mode):
>
> 0. When nothing is provided, DHCP is enabled and uses default
> (macOS-configured) settings.
>     Your interface can communicate with any other vmnet-host interface.
>
> 1. When provided `@isolated=3Don` only,  DHCP is still enabled, but the
> interface can
>    communicate only with the host. All the attempts to create another
> interface with
>    the same subnet will fail with `VMNET_SHARING_SERVICE_BUSY`. If we don't
>    specify DHCP settings (start, end, mask), macOS adjusts them
> automatically to
>    prevent collisions and the `VMNET_SHARING_SERVICE_BUSY` error.
>
> 2. When provided @net-uuid=3Duuid[,@isolated=3Doff], the interface can
> communicate to any
>      other interface inside the 'uuid' network. Also with other VMs under
> other hypervisors.
>      DHCP is disabled.
>
> 3. When provided @net-uuid=3Duuid,@isolated=3Don, the interface can communi=
> cate
> only with
>     the host.  DHCP is disabled. All the attempts to create another
> interface with
>     the @net-uuid will fail with `VMNET_SHARING_SERVICE_BUSY`.

The doc comments don't make this clear, I'm afraid.  Care to improve
them some?

>> When "no DCHCP service is provided", then @dhcpstart and @dhcpend become
>> misnomers.  Compare NetdevUserOptions: it uses @net to specify the
>> address range, and dhcpstart to specify the DHCP range (@dhcpend is
>> implied).  Should we aim for consistency between the two?
>>
>
> I think it is not our choice. All of these options are just mappings to
> vmnet interface
> params:
> https://developer.apple.com/documentation/vmnet/vmnet_constants

I think it *is* our choice.  We can choose to be internally consistent,
i.e. with NetdevUserOptions, or consistent with the external interface
we expose internally, i.e. with the vmnet interface.  Tradeoff.

> @subnet-mask -> vmnet_subnet_mask_key
> @dhcpstart -> vmnet_start_address_key
> @dhcpend ->  vmnet_end_address_key

Note that the right hand side does not say "DHCP".  I think the left
hand side should not, either.  Why not @start-address and @end-address?

> More detailed description is provided in the `vmnet.h` header of macOS SDK.
> There is a restriction that says: when the `vmnet_subnet_mask_key`
> (@subnet-mask)
> is set you "must also specify vmnet_start_address_key and
> vmnet_end_address_key".
> So, when "no DHCP service is provided" the`@subnet-mask` property also
> becomes
> a misnomer.

I don't think so.  vmnet_end_address_key, vmnet_end_address_key,
vmnet_subnet_mask_key all make sense regardless of DHCP service.

The gateway address is hardwired to the first address in the range.  If
DHCP is provided, it hands out addresses from the remainder of the
range.

> I added a corresponding restriction: you cannot provide @net-uuid within
> any of DHCP
> options, this causes hard failure with `error_setg()` (see vmnet-host.c).
>
>
>> +#
>> > +# Since: 6.2
>>
>> Missed 6.2, please adjust.  More of the same below.
>>
>>
> The next one is 6.3, isn't it?

7.0

>> +##
>> > +{ 'struct': 'NetdevVmnetHostOptions',
>> > +  'data': {
>> > +    '*dhcpstart':   'str',
>> > +    '*dhcpend':     'str',
>> > +    '*subnetmask':  'str',
>> > +    '*isolated':    'bool',
>> > +    '*net-uuid':    'str'
>> > +  },
>> > +  'if': 'CONFIG_VMNET' }
>> > +
>> > +##
>> > +# @NetdevVmnetSharedOptions:
>> > +#
>> > +# vmnet (shared mode) network backend.
>> > +#
>> > +# Allows traffic originating from the vmnet interface to reach the
>> > +# Internet through a network address translator (NAT). The vmnet
>> > +# interface can also communicate with the native host. By default,
>> > +# the vmnet interface is able to communicate with other shared mode
>> > +# interfaces. If a subnet range is specified, the vmnet interface can
>>
>> Do you mean "subnet mask"?
>>
>>
> This phrase was just copied from Apple's `vmnet.h` from SDK:
> ```
>  * [...] VMNET_SHARED_MODE
>  * Allows traffic originating from the vmnet interface to reach the
>  * Internet through a network address translator (NAT). The vmnet interface
>  * can also communicate with the native host. By default, the vmnet
> interface
>  * is able to communicate with other shared mode interfaces. If a subnet
> range
>  * is specified, the vmnet interface can communicate with other shared mode
>  * interfaces on the same subnet.
> ```
> But `subnet mask` sounds more suitable, so it's better to say:
> "... If a subnet mask and DHCP range is specified, the vmnet interface can
> ..."
>
>> +# communicate with other shared mode interfaces on the same subnet.
>> > +#
>> > +# @dhcpstart: The starting IPv4 address to use for the interface.
>> > +#             Must be in the private IP range (RFC 1918). Must be
>> > +#             specified along with @dhcpend and @subnetmask.
>> > +#             This address is used as the gateway address. The
>> > +#             subsequent address up to and including dhcpend are
>> > +#             placed in the DHCP pool.
>> > +#
>> > +# @dhcpend: The DHCP IPv4 range end address to use for the
>> > +#           interface. Must be in the private IP range (RFC 1918).
>> > +#           Must be specified along with @dhcpstart and @subnetmask.
>> > +#
>> > +# @subnetmask: The IPv4 subnet mask to use on the interface. Must
>> > +#              be specified along with @dhcpstart and @subnetmask.
>> > +#
>> > +# @isolated: Enable isolation for this interface. Interface isolation
>> > +#            ensures that network communication between multiple
>> > +#            vmnet interfaces instances is not possible.
>> > +#
>> > +# Since: 6.2
>> > +##
>> > +{ 'struct': 'NetdevVmnetSharedOptions',
>> > +  'data': {
>> > +    '*dhcpstart':    'str',
>> > +    '*dhcpend':      'str',
>> > +    '*subnetmask':   'str',
>> > +    '*isolated':     'bool'
>> > +  },
>> > +  'if': 'CONFIG_VMNET' }
>>
>> [...]
>
> As a conclusion, my TODOs for now:
>   * fix `subnet range` in documentation
>   * add dash to @subnet-mask
>   * change version: 6.2 -> 6.3
>   * provide IPv6 configuration properties for vmnet-shared (optional).