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.
