[prev in list] [next in list] [prev in thread] [next in thread] 

List:       netfilter-devel
Subject:    Re: [PATCH RFC 4/4] netfilter: nf_tables: add netlink description
From:       Johannes Berg <johannes () sipsolutions ! net>
Date:       2019-03-29 10:59:46
Message-ID: f882918e408247a4e0e8d0007feead5bbf311003.camel () sipsolutions ! net
[Download RAW message or body]

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.

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.

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



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?

johannes

[prev in list] [next in list] [prev in thread] [next in thread]