Hi Johannes,
2016-03-09, 12:34:23 +0100, Johannes Berg wrote:
> On Wed, 2016-03-09 at 11:51 +0100, Sabrina Dubroca wrote:
> > > > + MACSEC_ATTR_ICV_LEN,
> > > > + MACSEC_TXSA_LIST,
> > > > + MACSEC_RXSC_LIST,
> > > > + MACSEC_TXSC_STATS,
> > > > + MACSEC_SECY_STATS,
> > > > + MACSEC_ATTR_PROTECT,
> > >
> > > This seems a bit inconsistent, MACSEC_ATTR_* vs. MACSEC_*?
> >
> > Only the MACSEC_ATTR_* can be set, the others are just for dumping.
>
> Makes sense too.
>
> I tend to prefer the names having a consistent prefix to indicate the
> enum they're used in, which indicates the nesting level in nl80211 etc.
> and makes it easier to figure out in the code that they're used
> correctly (since accidentally mixing enums will give no warnings), but
> that's just personal preference I guess.
I see. I like the verification aspect, I'm adapting the enums now.
> > > > +enum macsec_sa_list_attrs {
> > > > + MACSEC_SA_LIST_UNSPEC,
> > > > + MACSEC_SA,
> > > > + __MACSEC_ATTR_SA_LIST_MAX,
> > > > + MACSEC_ATTR_SA_LIST_MAX = __MACSEC_ATTR_SA_LIST_MAX - 1,
> > > > +};
> > >
> > > Again, without documentation, it seems odd to have an enum with
> > > just a
> > > single useful entry? If you just wanted an array you don't need
> > > this at
> > > all? The netlink nesting properties could be specified somewhere.
> >
> > Yes, in dump_secy(), I nest the TXSA_LIST, and then each SA
> > underneath
> > it. I'm not sure how that would work without the list. Can you have
> > an array without the dummy level of nesting?
>
>
> So, if I understand correctly, your message would be
> [
> ..., /* e.g. IFINDEX, perhaps */
> TXSA_LIST -> [
> MACSEC_SA -> [
> MACSEC_ATTR_SA_AN -> ...,
> MACSEC_ATTR_SA_PN -> ...
> ],
> MACSEC_SA -> [...],
> MACSEC_SA -> [...],
> ...
> ],
> ]
>
> right? That seems pretty odd to me, usually the same nesting level in
> netlink shouldn't contain the same attribute multiple times, as I
> understand it.
Well, it worked ;)
> I *think* the way we do this in nl80211 is more customary, it would be
> like this in your case (without defining the sa_list_attrs enum):
>
> [
> ..., /* e.g. IFINDEX, perhaps */
> TXSA_LIST -> [
> 1 -> [
> MACSEC_ATTR_SA_AN -> ...,
> MACSEC_ATTR_SA_PN -> ...
> ],
> 2 -> [...],
> 3 -> [...],
> ...
> ],
> ]
>
> See, for example, nl80211_send_wowlan_patterns() which nests like this:
>
> [
> NL80211_WOWLAN_TRIG_PKT_PATTERN -> [
> 1 -> [
> NL80211_PKTPAT_MASK -> ...,
> NL80211_PKTPAT_PATTERN -> ...,
> NL80211_PKTPAT_OFFSET -> ...,
> ],
> 2 -> [...],
> ...
> ]
> ]
Ah, ok. I'm using this now, no more dummy enum. And thanks for the pointer!
> > These stats are defined by the standard, but marked optional.
> > A hardware device that doesn't implement some stat could just ignore
> > it and export 0.
>
> Fair enough. I tend to think there could be a difference between
> knowing the value was 0 and knowing it wasn't provided, particularly
> for the "exceptions" that you'd hope are mostly 0 under good operating
> conditions, but I don't have a strong opinion about these or,
> obviously, any idea about whether hardware might not be able to provide
> them.
Hmm, yeah, that makes sense. I'll think about it a bit more, maybe I
will change that before I resubmit. The separate attributes would
also help a bit in case we need to add more stats.
Thanks,
--
Sabrina
