On Fri, Mar 29, 2019 at 11:59:46AM +0100, Johannes Berg wrote:
> On Wed, 2018-02-07 at 02:37 +0100, Pablo Neira Ayuso wrote:
> >
> > +static const struct nl_desc_attr nft_nldesc_table_attrs[NFTA_TABLE_MAX + 1] = {
> > + NLDESC_ATTR_STRING(NFTA_TABLE_NAME, NFT_NAME_MAXLEN - 1),
> > + NLDESC_ATTR_U32_MAX(NFTA_TABLE_FLAGS, NFT_TABLE_F_DORMANT),
> > + NLDESC_ATTR_U32(NFTA_TABLE_USE),
> > + NLDESC_ATTR_U64(NFTA_TABLE_HANDLE),
> > + NLDESC_ATTR_PAD(NFTA_TABLE_PAD),
> > +};
>
> This part I don't really like so much. Yes, it's >1yo, so we didn't have
> all the stuff at the time.
I'm glad to receive feedback, even 1 year later :-)
> But this is _lots_ of code, and most of it is already encoded in the
> nla_policy for all those attributes. I think we really need to find a
> way to unify the two sets of data.
We could probably extract this from the nla_policy if we extend it. I
can have a look...
> I'd actually be happy to *just* expose the policy with all its nested
> elements/objects in there. That'd probably be also almost enough for
> you?
Exposing commands is also important, I know of people poking for
commands to see if they are available or not.
I think we should full description to userspace, not only policies.
> And that way you don't need to maintain the same descriptions twice
> (and we *will* get it wrong if we do that).
>
> For genl sub-families, I'd also say that the whole thing could be
> exposed within the genl family, so I think we'd want to express this as
> some kind of helper logic to export the policy data.
>
> I'm not entirely sure what the whole story with your "objects" is
> though. Aren't those just nested attributes of sort?
>
> I see e.g.
>
> >
> > +static const struct nl_desc_attr nft_nldesc_set_desc_attrs[NFTA_SET_DESC_MAX + 1] = {
> > + NLDESC_ATTR_U32(NFTA_SET_DESC_SIZE),
> > +};
>
> This is policy data - like I said above we should unify the two.
>
> > +static const struct nl_desc_obj nft_nldesc_set_desc[] = {
> > + NLDESC_OBJ(NFT_SET_DESC, nft_nldesc_set_desc_attrs, NFTA_SET_DESC_MAX),
>
> This is some kind of "object", which I'm not sure I understand, but
> NFTA_SET_DESC_MAX is just the policy data of what the max attribute is
> for this thing.
>
> > +static const struct nl_desc_attr nft_nldesc_set_attrs[NFTA_SET_MAX + 1] = {
> ...
> > + NLDESC_ATTR_NESTED(NFTA_SET_DESC, nft_nldesc_set_desc),
>
> and here this is again just a policy thing, more or less.
>
> So it seems to me that without even having an object enum, we'd get the
> same kind of data by
>
> struct nla_policy nft_policy_set_desc[NFTA_SET_DESC_MAX + 1] = {
> [NFTA_SET_DESC_SIZE] = { .type = NLA_U32 },
> };
>
> struct nla_policy nft_policy_set[NFTA_SET_MAX + 1] = {
> [NFTA_SET_DESC] = NLA_POLICY_NESTED(nft_policy_set_desc),
> };
>
> Except we don't have an "object ID" (and obviously also that we can use
> the same data to actually parse/validate messages, and so don't end up
> with bad situations of nldesc saying one thing and the policy another.)
The object ID is needed by userspace, to build a flat table, and look
up for the nested IDs, to express the graph. We can probably place
this in the policy.
> So ... Assuming I write some code to expose the policy (which I plan on
> doing), what would this still be able to expose in addition?
This would be incomplete, since commands are an essential part too,
and relation of attributes with commands are also too.
