Re: [libnftnl RFC 2/2] expr: Introduce struct expr_ops::attr_policy

[Florian Westphal <fw@strlen.de>]

Phil Sutter <phil@nwl.cc> wrote:
> Similar to kernel's nla_policy, enable expressions to inform about
> restrictions on attribute use. This allows the generic expression code
> to perform sanity checks before dispatching to expression ops.
> 
> For now, this holds only the maximum data len which may be passed to
> nftnl_expr_set().
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/expr_ops.h   |  5 +++++
>  src/expr.c           |  6 ++++++
>  src/expr/bitwise.c   | 11 +++++++++++
>  src/expr/byteorder.c |  9 +++++++++
>  src/expr/immediate.c |  9 +++++++++
>  5 files changed, 40 insertions(+)
> 
> diff --git a/include/expr_ops.h b/include/expr_ops.h
> index 51b221483552c..6c95297bfcd58 100644
> --- a/include/expr_ops.h
> +++ b/include/expr_ops.h
> @@ -8,10 +8,15 @@ struct nlattr;
>  struct nlmsghdr;
>  struct nftnl_expr;
>  
> +struct attr_policy {
> +	size_t maxlen;

I'd make this an uint32_t since that is also whats
passed to expr_set().

> +		if (expr->ops->attr_policy &&
> +		    type <= expr->ops->nftnl_max_attr &&
> +		    expr->ops->attr_policy[type].maxlen &&
> +		    expr->ops->attr_policy[type].maxlen < data_len)
> +			return -1;
> +

I'd make this more strict, is there a reason to call ops->set
if type > ->nftnl_max_attr?

Something like:

!attr_policy -> return -1;
type > nftnl_max_attr -> return -1:
data_len > maxlen -> return -1.

We could also define a minlen to disallow setting
e.g. 1 byte of something that is internally an u32.

But I admit that this might break compatibility
or otherwise leak internals into the api.