Re: [PATCH nft 3/9] datatype: drop flags field from datatype

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

On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote:
> Flags are not always bad. For example, as a function argument they allow
> easier extension in the future. But with datatype's "flags" argument and
> enum datatype_flags there are no advantages of this approach.
> 
> - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This could even
>   be a bool:1 bitfield if we cared to represent the information with
>   one bit only. For now it's not done because that would not help reducing
>   the size of the struct, so a bitfield is less preferable.
> 
> - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent static
>   instances. Drop this redundant flag.

Not sure I want to rely on refcnt to zero to identify dynamic
datatypes. I think we need to consolidate datatype_set() to be used
not only where this deals with dynamic datatypes, it might help
improve traceability of datatype assignment.

> - move the integer field "refcnt" in struct datatype beside other fields
>   of similar size/alignment. This makes the size of the struct by one
>   pointer size smaller (on x86-64).
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/datatype.h        | 24 +++++++++---------------
>  src/datatype.c            | 20 ++++++++------------
>  src/meta.c                |  2 +-
>  src/netlink_delinearize.c |  2 +-
>  src/rt.c                  |  2 +-
>  src/segtree.c             |  5 ++---
>  6 files changed, 22 insertions(+), 33 deletions(-)
> 
> diff --git a/include/datatype.h b/include/datatype.h
> index 52a2e943b252..5b85adc15857 100644
> --- a/include/datatype.h
> +++ b/include/datatype.h
> @@ -120,24 +120,13 @@ enum byteorder {
>  
>  struct expr;
>  
> -/**
> - * enum datatype_flags
> - *
> - * @DTYPE_F_ALLOC:		datatype is dynamically allocated
> - * @DTYPE_F_PREFIX:		preferred representation for ranges is a prefix
> - */
> -enum datatype_flags {
> -	DTYPE_F_ALLOC		= (1 << 0),
> -	DTYPE_F_PREFIX		= (1 << 1),
> -};
> -
>  struct parse_ctx;
>  /**
>   * struct datatype
>   *
>   * @type:	numeric identifier
>   * @byteorder:	byteorder of type (non-basetypes only)
> - * @flags:	flags
> + * @f_prefix:	preferred representation for ranges is a prefix
>   * @size:	type size (fixed sized non-basetypes only)
>   * @subtypes:	number of subtypes (concat type)
>   * @name:	type name
> @@ -147,14 +136,20 @@ struct parse_ctx;
>   * @print:	function to print a constant of this type
>   * @parse:	function to parse a symbol and return an expression
>   * @sym_tbl:	symbol table for this type
> - * @refcnt:	reference counter (only for DTYPE_F_ALLOC)
> + * @refcnt:	reference counter for dynamically allocated instances.
>   */
>  struct datatype {
>  	uint32_t			type;
>  	enum byteorder			byteorder;
> -	unsigned int			flags;
> +	bool				f_prefix;
>  	unsigned int			size;
>  	unsigned int			subtypes;
> +
> +	/* Refcount for dynamically allocated instances. For static instances
> +	 * this is zero (get() and free() are NOPs).
> +	 */
> +	unsigned int			refcnt;
> +
>  	const char			*name;
>  	const char			*desc;
>  	const struct datatype		*basetype;
> @@ -169,7 +164,6 @@ struct datatype {
>  	struct error_record		*(*err)(const struct expr *sym);
>  	void				(*describe)(struct output_ctx *octx);
>  	const struct symbol_table	*sym_tbl;
> -	unsigned int			refcnt;
>  };
>  
>  extern const struct datatype *datatype_lookup(enum datatypes type);
> diff --git a/src/datatype.c b/src/datatype.c
> index 70c84846f70e..c5d88d9a90b6 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -641,7 +641,7 @@ const struct datatype ipaddr_type = {
>  	.basetype	= &integer_type,
>  	.print		= ipaddr_type_print,
>  	.parse		= ipaddr_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  static void ip6addr_type_print(const struct expr *expr, struct output_ctx *octx)
> @@ -708,7 +708,7 @@ const struct datatype ip6addr_type = {
>  	.basetype	= &integer_type,
>  	.print		= ip6addr_type_print,
>  	.parse		= ip6addr_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  static void inet_protocol_type_print(const struct expr *expr,
> @@ -944,7 +944,7 @@ const struct datatype mark_type = {
>  	.print		= mark_type_print,
>  	.json		= mark_type_json,
>  	.parse		= mark_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  static const struct symbol_table icmp_code_tbl = {
> @@ -1203,9 +1203,7 @@ static struct datatype *datatype_alloc(void)
>  	struct datatype *dtype;
>  
>  	dtype = xzalloc(sizeof(*dtype));
> -	dtype->flags = DTYPE_F_ALLOC;
>  	dtype->refcnt = 1;
> -
>  	return dtype;
>  }
>  
> @@ -1215,10 +1213,10 @@ struct datatype *datatype_get(const struct datatype *ptr)
>  
>  	if (!dtype)
>  		return NULL;
> -	if (!(dtype->flags & DTYPE_F_ALLOC))
> -		return dtype;
>  
> -	dtype->refcnt++;
> +	if (dtype->refcnt > 0)
> +		dtype->refcnt++;
> +
>  	return dtype;
>  }
>  
> @@ -1245,7 +1243,6 @@ struct datatype *datatype_clone(const struct datatype *orig_dtype)
>  	*dtype = *orig_dtype;
>  	dtype->name = xstrdup(orig_dtype->name);
>  	dtype->desc = xstrdup(orig_dtype->desc);
> -	dtype->flags = DTYPE_F_ALLOC | orig_dtype->flags;
>  	dtype->refcnt = 1;
>  
>  	return dtype;
> @@ -1257,10 +1254,9 @@ void datatype_free(const struct datatype *ptr)
>  
>  	if (!dtype)
>  		return;
> -	if (!(dtype->flags & DTYPE_F_ALLOC))
> -		return;
>  
> -	assert(dtype->refcnt != 0);
> +	if (dtype->refcnt == 0)
> +		return;
>  
>  	if (--dtype->refcnt > 0)
>  		return;
> diff --git a/src/meta.c b/src/meta.c
> index 181e111cbbdc..7bf749b34fb4 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -368,7 +368,7 @@ const struct datatype devgroup_type = {
>  	.print		= devgroup_type_print,
>  	.json		= devgroup_type_json,
>  	.parse		= devgroup_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  const struct datatype ifname_type = {
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index 41cb37a3ccb3..f3939be2d063 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -2568,7 +2568,7 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
>  		default:
>  			break;
>  		}
> -	} else if (binop->left->dtype->flags & DTYPE_F_PREFIX &&
> +	} else if (binop->left->dtype->f_prefix &&
>  		   binop->op == OP_AND && expr->right->etype == EXPR_VALUE &&
>  		   expr_mask_is_prefix(binop->right)) {
>  		expr->left = expr_get(binop->left);
> diff --git a/src/rt.c b/src/rt.c
> index 9ddcb210eaad..ccea0aa9bc44 100644
> --- a/src/rt.c
> +++ b/src/rt.c
> @@ -55,7 +55,7 @@ const struct datatype realm_type = {
>  	.basetype	= &integer_type,
>  	.print		= realm_type_print,
>  	.parse		= realm_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  const struct rt_template rt_templates[] = {
> diff --git a/src/segtree.c b/src/segtree.c
> index 0a12a0cd5151..637457b087b9 100644
> --- a/src/segtree.c
> +++ b/src/segtree.c
> @@ -402,8 +402,7 @@ void concat_range_aggregate(struct expr *set)
>  				goto next;
>  			}
>  
> -			if (prefix_len < 0 ||
> -			    !(r1->dtype->flags & DTYPE_F_PREFIX)) {
> +			if (prefix_len < 0 || !r1->dtype->f_prefix) {
>  				tmp = range_expr_alloc(&r1->location, r1,
>  						       r2);
>  
> @@ -518,7 +517,7 @@ add_interval(struct expr *set, struct expr *low, struct expr *i)
>  		expr = expr_get(low);
>  	} else if (range_is_prefix(range) && !mpz_cmp_ui(p, 0)) {
>  
> -		if (i->dtype->flags & DTYPE_F_PREFIX)
> +		if (i->dtype->f_prefix)
>  			expr = interval_to_prefix(low, i, range);
>  		else if (expr_basetype(i)->type == TYPE_STRING)
>  			expr = interval_to_string(low, i, range);
> -- 
> 2.41.0
>