Hi Johannes,

2016-03-09, 12:34:23 +0100, Johannes Berg wrote:

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

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

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