Re: [PATCH net-next 1/3] uapi: add MACsec bits

[Sabrina Dubroca <sd@queasysnail.net>]

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