Thank you for your review.
On Thu, Jun 17, 2021 at 05:32:41PM +0300, Vladislav Yaroshchuk wrote:
> Created separate netdev per each vmnet operating mode
> because they use quite different settings. Especially since
> macOS 11.0 (vmnet.framework API gets lots of updates)
>
> Three new netdevs are added:
> - vmnet-host
> - vmnet-shared
> - vmnet-bridged
>
> Signed-off-by: Vladislav Yaroshchuk <yaroshchuk2000@gmail.com>
> ---
> +++ b/qapi/net.json
> @@ -452,6 +452,89 @@
> '*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.
> +#
> +# @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.
> +#
> +#
> +# Since: 6.1,
> +##
Same comments about 6.1 vs. 6.2 as before (I'll quit pointing it out).
Spurious trailing comma.
> +{ 'struct': 'NetdevVmnetHostOptions',
> + 'data': {
> + '*dhcpstart': 'str',
> + '*dhcpend': 'str',
> + '*subnetmask': 'str'
Hmm. You listed all three as optional, but document that they must all
be specified together. Why not just make them all required, and
simplify the documentation?
All three options can be not specified at all, or, if specified, must be used all together. It's the user's choice to adjust DHCP settings or leave it for vmnet.framework.
`-netdev vmnet-host` is correct and `-netdev vmnet-host,dhcpstart="..",dhcpend="..",subnetmask=".."` is correct too. So we can't make all the options required
> + },
> + 'if': 'defined(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 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.
Spurious double space.
> +#
> +# @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.
> +#
> +#
> +# Since: 6.1,
> +##
> +{ 'struct': 'NetdevVmnetSharedOptions',
> + 'data': {
> + '*dhcpstart': 'str',
> + '*dhcpend': 'str',
> + '*subnetmask': 'str'
> + },
> + 'if': 'defined(CONFIG_VMNET)' }
> +
> +##
> +# @NetdevVmnetBridgedOptions:
> +#
> +# vmnet (bridged mode) network backend.
> +#
> +# Bridges the vmnet interface with a physical network interface.
> +#
> +# @ifname: The name of the physical interface to be bridged.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'NetdevVmnetBridgedOptions',
> + 'data': { 'ifname': 'str' },
> + 'if': 'defined(CONFIG_VMNET)' }
> +
> ##
> # @NetClientDriver:
> #
> @@ -460,11 +543,16 @@
> # Since: 2.7
> #
> # @vhost-vdpa since 5.1
> -# @vmnet since 6.1
Why are you dropping vmnet? Especially since you just added it in the
previous patch? That feels like needless churn.
Yep, that was my mistake, on that stage I decided to create separate backends for each vmnet operational mode. Will remove redundant change the next series.
> +# @vmnet-host since 6.1
> +# @vmnet-shared since 6.1
> +# @vmnet-bridged since 6.1
> ##
> { 'enum': 'NetClientDriver',
> 'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> - 'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa', 'vmnet' ] }
> + 'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa',
> + { 'name': 'vmnet-host', 'if': 'defined(CONFIG_VMNET)' },
> + { 'name': 'vmnet-shared', 'if': 'defined(CONFIG_VMNET)' },
> + { 'name': 'vmnet-bridged', 'if': 'defined(CONFIG_VMNET)' }] }
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Will fix all other issues asap, thank you!
Regards,
Vladislav