Re: [PATCH conntrack-tools v2 1/2] conntrack: move label parsing after argument parsing

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

Hi Christoph,

On Tue, Jun 17, 2025 at 12:48:33PM +0200, Christoph Heiss wrote:
> Instead of parsing directly inline while parsing, put them into a list
> and do it afterwards.
> 
> Preparation for introduction a new `--labelmap` option to specify the
> path to the label mapping file.

Just a few cosmetic nitpicks on my side.

> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  src/conntrack.c | 60 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/src/conntrack.c b/src/conntrack.c
> index 2d4e864..b9afd2f 100644
> --- a/src/conntrack.c
> +++ b/src/conntrack.c
> @@ -122,6 +122,12 @@ struct ct_cmd {
>  	struct ct_tmpl	tmpl;
>  };
>  
> +struct ct_label {
> +	struct list_head list;
> +	char *name;
> +	bool is_modify;
> +};
> +
>  static int alloc_tmpl_objects(struct ct_tmpl *tmpl)
>  {
>  	tmpl->ct = nfct_new();
> @@ -2963,6 +2969,30 @@ static int print_stats(const struct ct_cmd *cmd)
>  	return 0;
>  }
>  
> +static void parse_and_merge_labels(struct list_head *labels, struct ct_tmpl *tmpl)
> +{
> +	struct ct_label *l, *next;
        struct nfct_bitmask *b;
        unsigned int max;

reverse xmas tree in variable declaration

and line break here after variable declaration block.

I would suggest these variable names:

- label_list instead of labels.
- label instead of l.

the short variable name 'l' usually makes it harder to search for
variables in my editor.

> +	list_for_each_entry_safe(l, next, labels, list) {
> +		unsigned int max = parse_label_get_max(l->name);
> +		struct nfct_bitmask *b = nfct_bitmask_new(max);

> +		if (!b)
> +			exit_error(OTHER_PROBLEM, "out of memory");
> +
> +		parse_label(b, l->name);
> +
> +		/* join "-l foo -l bar" into single bitmask object */
> +		if (l->is_modify) {
> +			merge_bitmasks(&tmpl->label_modify, b);
> +		} else {
> +			merge_bitmasks(&tmpl->label, b);
> +		}

For single statement:

		if (l->is_modify)
			merge_bitmasks(&tmpl->label_modify, b);
		else
			merge_bitmasks(&tmpl->label, b);

Just cosmetic stuff, I hope not to bother you and Florian too much
with this.

> +
> +		list_del(&l->list);
> +		free(l->name);
> +		free(l);
> +	}
> +}
> +
>  static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  {
>  	unsigned int type = 0, event_mask = 0, l4flags = 0, status = 0;
> @@ -2973,6 +3003,7 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  	struct ct_tmpl *tmpl;
>  	int res = 0, partial;
>  	union ct_address ad;
> +	LIST_HEAD(labels);
>  	uint32_t value;
>  	int c, cmd;
>  
> @@ -3088,8 +3119,6 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  		case 'o':
>  			options |= CT_OPT_OUTPUT;
>  			parse_parameter(optarg, &output_mask, PARSE_OUTPUT);
> -			if (output_mask & _O_CL)
> -				labelmap_init();
>  			if ((output_mask & _O_SAVE) &&
>  			    (output_mask & (_O_EXT |_O_TMS |_O_ID | _O_KTMS | _O_CL | _O_XML)))
>  				exit_error(OTHER_PROBLEM,
> @@ -3162,8 +3191,6 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  		case '>':
>  			options |= opt2type[c];
>  
> -			labelmap_init();
> -
>  			if ((options & (CT_OPT_DEL_LABEL|CT_OPT_ADD_LABEL)) ==
>  			    (CT_OPT_DEL_LABEL|CT_OPT_ADD_LABEL))
>  				exit_error(OTHER_PROBLEM, "cannot use --label-add and "
> @@ -3176,22 +3203,13 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  				optarg = tmp;
>  			}
>  
> -			char *optarg2 = strdup(optarg);
> -			unsigned int max = parse_label_get_max(optarg);
> -			struct nfct_bitmask * b = nfct_bitmask_new(max);
> -			if (!b)
> +			struct ct_label *l = calloc(1, sizeof(*l));
> +			if (!l)
>  				exit_error(OTHER_PROBLEM, "out of memory");
>  
> -			parse_label(b, optarg2);
> -
> -			/* join "-l foo -l bar" into single bitmask object */
> -			if (c == 'l') {
> -				merge_bitmasks(&tmpl->label, b);
> -			} else {
> -				merge_bitmasks(&tmpl->label_modify, b);
> -			}
> -
> -			free(optarg2);
> +			l->name = strdup(optarg);
> +			l->is_modify = c == '<' || c == '>';
> +			list_add_tail(&l->list, &labels);
>  			break;
>  		case 'a':
>  			fprintf(stderr, "WARNING: ignoring -%c, "
> @@ -3246,6 +3264,12 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  		}
>  	}
>  
> +	/* any of these options (might) use labels */
> +	if ((options & (CT_OPT_LABEL | CT_OPT_ADD_LABEL | CT_OPT_DEL_LABEL)) ||
> +	    ((options & CT_OPT_OUTPUT) && (output_mask & _O_CL))) {
> +		labelmap_init();
> +		parse_and_merge_labels(&labels, tmpl);
> +	}
>  
>  	/* we cannot check this combination with generic_opt_check. */
>  	if (options & CT_OPT_ANY_NAT &&
> -- 
> 2.49.0
> 
> 
>