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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 20 Nov 2019 12:24:48 +0100
Phil Sutter <phil@nwl.cc> wrote:

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

Oh, yes, better. I'll change this in v2.

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

It won't change the input value at all, but that's not the point: I'm
reading from an array of 8-bit values and attributes are 32 bits. If I
htonl() directly on the input array, it's going to use 24 bits around
those 8 bits.

-- 
Stefano