Re: [PATCH libnftnl 3/3] desc: add set description

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Mar 10, 2022 at 12:31:15PM +0100, Phil Sutter wrote:
> Hi,
> 
> On Thu, Jan 20, 2022 at 01:04:02AM +0100, Pablo Neira Ayuso wrote:
> [...]
> > diff --git a/src/desc.c b/src/desc.c
> > index f73e74c2c7d3..c8b3195db850 100644
> > --- a/src/desc.c
> > +++ b/src/desc.c
> [...]
> > +static int nftnl_set_desc_build_dtype(struct nftnl_udata_buf *udbuf,
> > +				      const struct nftnl_set_desc *dset)
> > +{
> > +	struct nftnl_udata *nest;
> > +	int i, err;
> > +
> > +	switch (dset->type) {
> > +	case NFTNL_DESC_SET_TYPEOF:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY);
> > +		for (i = 0; i < dset->key.num_type; i++) {
> > +			err = __nftnl_udata_set_dtype_build(udbuf, dset->key.dtype[i], i);
> > +			if (err < 0)
> > +				return err;
> > +		}
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_DATATYPE:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA);
> > +		for (i = 0; i < dset->data.num_type; i++) {
> > +			err = __nftnl_udata_set_dtype_build(udbuf, dset->data.dtype[i], i);
> > +			if (err < 0)
> > +				return err;
> > +		}
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_UNSPEC:
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> > +					 const struct nftnl_expr_desc *dexpr,
> > +					 uint8_t attr_type)
> > +{
> > +	struct nftnl_udata *nest;
> > +	int err;
> > +
> > +	nest = nftnl_udata_nest_start(udbuf, attr_type);
> > +	err = nftnl_expr_desc_build(udbuf, dexpr);
> > +	nftnl_udata_nest_end(udbuf, nest);
> > +
> > +	return err;
> > +}
> > +
> > +static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> > +				       const struct nftnl_set_desc *dset)
> > +{
> > +	struct nftnl_udata *nest;
> > +	int i;
> > +
> > +	switch (dset->type) {
> > +	case NFTNL_DESC_SET_TYPEOF:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
> > +		for (i = 0; i < dset->key.num_typeof; i++)
> > +			__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);
> > +
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_DATATYPE:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA_TYPEOF);
> > +		for (i = 0; i < dset->key.num_typeof; i++)
> > +			__nftnl_set_desc_build_typeof(udbuf, dset->data.expr[i], i);
> > +
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_UNSPEC:
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +EXPORT_SYMBOL(nftnl_set_desc_build_udata);
> > +int nftnl_set_desc_build_udata(struct nftnl_udata_buf *udbuf,
> > +			       const struct nftnl_set_desc *dset)
> > +{
> > +	if (!nftnl_udata_put_u32(udbuf, NFTNL_UDATA_SET_FLAGS, dset->flags))
> > +		return -1;
> > +
> > +	switch (dset->type) {
> > +	case NFTNL_DESC_SET_DATATYPE:
> > +		return nftnl_set_desc_build_dtype(udbuf, dset);
> > +	case NFTNL_DESC_SET_TYPEOF:
> > +		return nftnl_set_desc_build_typeof(udbuf, dset);
> > +	case NFTNL_DESC_SET_UNSPEC:
> > +		return -1;
> > +	}
> > +
> > +	if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_SET_COMMENT, dset->comment))
> > +		return -1;
> > +
> > +	return -1;
> > +}
> 
> This is odd: Depending on dset->type, nftnl_set_desc_build_udata() calls
> either nftnl_set_desc_build_dtype() or nftnl_set_desc_build_typeof().

That's indeed incorrect, nftnl_set_desc_build_typeof() should be:

static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
				       const struct nftnl_set_desc *dset)
{
	struct nftnl_udata *nest;
	int i;

	nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
	for (i = 0; i < dset->key.num_typeof; i++)
		__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);

	nftnl_udata_nest_end(udbuf, nest);

	return 0;
}

The idea is: A set can either use datatype or typeof to define the
elements that it stores. Then, a concatenation is possible.

> Yet both check dset->type again. This looks like a mix-up of set/map key
> and data definitions and typeof vs. "regular" definition styles.

This patchset was a sketch PoC, I should have label it more explicit
as such.