Re: [Qemu-devel] [PATCH v2 2/2] net: introduce command to query mac-table information

[Amos Kong]

On Thu, May 16, 2013 at 09:38:01AM -0600, Eric Blake wrote:
> On 05/16/2013 05:07 AM, Amos Kong wrote:
> > We want to implement mac programming over macvtap through Libvirt.
> > The previous patch adds QMP event to notify management of mac-table
> > change. This patch adds a monitor command to query rx mode information
> > of mac-tables.
> > 
> 
> Focusing my review on just the QMP interface this go-around:
> 
> > +++ b/qapi-schema.json
> > @@ -3619,3 +3619,60 @@
> >              '*cpuid-input-ecx': 'int',
> >              'cpuid-register': 'X86CPURegister32',
> >              'features': 'int' } }
> > +
> > +# @MacTableInfo:
> > +#
> > +# Rx-mode information of mac-table for a net client.
> > +#
> > +# @name: the net client name
> > +#
> > +# @promisc: #optional promiscuous mode (default: false)
> > +#
> > +# @allmulti: #optional all multicast mode (default: false)
> > +#
> > +# @alluni: #optional all unicast mode (default: false)
> > +#
> > +# @nomulti: #optional no multicast mode (default: false)
> 
> Are allmulti and numulti orthogonal (all four pairings of possible), or
> are they tied together (tri-state)?

allmulti: receive all multicast packets
nomulti: don't receive multicast packets
normal: only receive multicast (in mac-table) packets

> If the latter, then maybe it's
> better to have an enum value for the three states (none, normal, all)
> and a singe @multi that resolves to one of those three states.

Sounds good. I frankly output the raw RX filter controls without
process & integrate.
 
> We don't necessarily need to abbreviate; would it be better giving these
> fields a longer name, such as no-multicast?

I used the abbreviations because they are same as parameters of config
tool. Management don't need this reminder, I will use no-multicast.
 
> > +#
> > +# @nouni: #optional no unicast mode (default: false)
> 
> Same orthogonal vs. tri-state question about alluni/nouni.
> 
> > +#
> > +# @nobcast: #optional no broadcast mode (default: false)
> 
> Again, if we don't abbreviate, should this be no-broadcast?
> 
> Double negatives can be awkward to work with; is it better to name this
> 'broadcast-allowed' with true being the default?

Ok.
 
> Is the point of labeling these fields #optional so that you can avoid
> emitting them if they are in their default state?

No.

Currently we only use rx-filter for virtio-nic, some rx-filer may
not be used by other emulated nic, so I used optional.

option 1:
   nic doesn't have the rx filter, nothing emitted (default :false)
   nic have the rx filter, emit the real value no matter it's true/falue

option 2:
   only emit when nic has the rx filter and value is true

option 2 should be right.

> Does it hurt to
> always list all fields, instead of omitting ones in their default state?
> 
> > +#
> > +# @multi_overflow: #optional multicast overflow mode (default: false)
 
...
 
> > +- "promisc": promiscuous mode (json-bool, optional)
> > +- "allmulti": all multicast mode (json-bool, optional)
> > +- "alluni": all unicast mode (json-bool, optional)
> > +- "nomulti":no multicast mode (json-bool, optional)
> > +- "nouni": no unicast mode (json-bool, optional)
> > +- "nobcast": no broadcast mode (json-bool, optional)
> > +- "multi-overflow": multicast overflow mode (json-bool, optional)
> > +- "uni-overflow": unicast overflow mode (json-bool, optional)
> 
> For all of the optional bools, please document what the default is if
> the field is omitted.  Or maybe we should just always emit them.

Yes, emit them all the time. If nic doesn't have the rx-filter, just set
it to default 'false'.
 
> > +- "unicast": a jason-array of unicast macaddr string (optional)
> 
> s/jason/json/
> 
> Isn't it better to omit a 0-length array when there is explicitly no
> unicast MAC registered, rather than omitting the field?  An omitted
> field implies that there is not enough information available to decide
> how many unicast addresses are registered.

So will remove the optional for unicast/multicast
 
> > +- "multicast": a jason-array of multicast macaddr string (optional)
> 
> Likewise on preferring a 0-length array rather than omitting the field
> altogether.
> 
> Looking forward to v2.

Thanks.

-- 
                        Amos.