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

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2016-03-09 at 11:51 +0100, Sabrina Dubroca wrote:

> > > +#define DEFAULT_CIPHER_NAME "GCM-AES-128"
> > > +#define DEFAULT_CIPHER_ID   0x0080020001000001ULL
> > > +#define DEFAULT_CIPHER_ALT  0x0080C20001000001ULL
> > 
> > > +enum macsec_attrs {
> > [...]
> > > +	MACSEC_ATTR_CIPHER_SUITE,
> > 
> > should that be the ID, the alternate ID, or the string?
> 
> uh, the string is never actually used, I could get rid of it.

Heh, ok. I was actually wondering about the string but didn't look up
where/if it was used :)

> > > +	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.

> > > +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.

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 -> [...],
        ...
    ]
]

> 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.

johannes