On Wed, 20 Nov 2019 12:24:48 +0100
Phil Sutter <phil@xxxxxx> 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
