Phillip Tennen <phillip.en...@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 <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. 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?