Re: [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes

From: Phil Sutter <phil@nwl.cc>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: "Pablo Neira Ayuso" <pablo@netfilter.org>,
	netfilter-devel@vger.kernel.org,
	"Florian Westphal" <fw@strlen.de>,
	"Kadlecsik József" <kadlec@blackhole.kfki.hu>,
	"Eric Garver" <eric@garver.life>
Subject: Re: [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes
Date: Wed, 20 Nov 2019 12:24:48 +0100	[thread overview]
Message-ID: <20191120112448.GI8016@orbyte.nwl.cc> (raw)
In-Reply-To: <20191119010723.39368-1-sbrivio@redhat.com>

Hi,

On Tue, Nov 19, 2019 at 02:07:23AM +0100, Stefano Brivio wrote:
[...]
> diff --git a/src/set.c b/src/set.c
> index 78447c6..60a46d8 100644
> --- a/src/set.c
> +++ b/src/set.c
[...]
> @@ -361,6 +366,23 @@ nftnl_set_nlmsg_build_desc_payload(struct nlmsghdr *nlh, struct nftnl_set *s)
>  	mnl_attr_nest_end(nlh, nest);
>  }
>  
> +static void
> +nftnl_set_nlmsg_build_subkey_payload(struct nlmsghdr *nlh, struct nftnl_set *s)
> +{
> +	struct nlattr *nest;
> +	uint32_t v;
> +	uint8_t *l;
> +
> +	nest = mnl_attr_nest_start(nlh, NFTA_SET_SUBKEY);
> +	for (l = s->subkey_len; l - s->subkey_len < NFT_REG32_COUNT; l++) {

While I like pointer arithmetics, too, I don't think it's much use here.
Using good old index variable even allows to integrate the zero value
check:

|	for (i = 0; i < NFT_REG32_COUNT && s->subkey_len[i]; i++)

> +		if (!*l)
> +			break;
> +		v = *l;
> +		mnl_attr_put_u32(nlh, NFTA_SET_SUBKEY_LEN, htonl(v));

I guess you're copying the value here because how htonl() is declared,
but may it change the input value non-temporarily? I mean, libnftnl is
in control over the array so from my point of view it should be OK to
directly pass it to htonl().

Cheers, Phil