Re: [PATCH conntrack-tools] conntrack: introduce --labelmap option to specify connlabel.conf path

[Florian Westphal <fw@strlen.de>]

Christoph Heiss <c.heiss@proxmox.com> wrote:
> Enables specifying a path to a connlabel.conf to load instead of the
> default one at /etc/xtables/connlabel.conf.
> 
> nfct_labelmap_new() already allows supplying a custom path to load
> labels from, so it just needs to be passed in there.

Makes sense, patch looks good to me.

>  3 files changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/conntrack.8 b/conntrack.8
> index 3b6a15b..2b6da25 100644
> --- a/conntrack.8
> +++ b/conntrack.8
> @@ -189,6 +189,13 @@ This option is only available in conjunction with "\-L, \-\-dump",
>  Match entries whose labels include those specified as arguments.
>  Use multiple \-l options to specify multiple labels that need to be set.
>  .TP
> +.BI "--labelmap " "PATH"
> +Specify the path to a connlabel.conf file to load instead of the default one.
> +This option is only available in conjunction with "\-L, \-\-dump", "\-E,
> +\-\-event", "\-U \-\-update" or "\-D \-\-delete". Must come before any of
> +"\-l, \-\-label", "\-\-label\-add" or "\-\-label\-del", otherwise the argument is
> +ignored.
> +.TP
>  .BI "--label-add " "LABEL"
>  Specify the conntrack label to add to the selected conntracks.
>  This option is only available in conjunction with "\-I, \-\-create",
> diff --git a/include/conntrack.h b/include/conntrack.h
> index 6dad4a1..317cab6 100644
> --- a/include/conntrack.h
> +++ b/include/conntrack.h
> @@ -78,7 +78,7 @@ enum ct_command {
>  };
>  
>  #define NUMBER_OF_CMD   _CT_BIT_MAX
> -#define NUMBER_OF_OPT   29
> +#define NUMBER_OF_OPT   30
>  
>  struct nf_conntrack;
>  
> diff --git a/src/conntrack.c b/src/conntrack.c
> index 2d4e864..9850825 100644
> --- a/src/conntrack.c
> +++ b/src/conntrack.c
> @@ -249,6 +249,9 @@ enum ct_options {
>  
>  	CT_OPT_REPL_ZONE_BIT	= 28,
>  	CT_OPT_REPL_ZONE	= (1 << CT_OPT_REPL_ZONE_BIT),
> +
> +	CT_OPT_LABELMAP_BIT	= 29,
> +	CT_OPT_LABELMAP		= (1 << CT_OPT_LABELMAP_BIT),

Why is this needed?

> +static char *labelmap_path;
>  static struct nfct_labelmap *labelmap;
>  static int filter_family;
>  
> @@ -2756,7 +2764,7 @@ static void labelmap_init(void)
>  {
>  	if (labelmap)
>  		return;
> -	labelmap = nfct_labelmap_new(NULL);
> +	labelmap = nfct_labelmap_new(labelmap_path);
>  	if (!labelmap)
>  		perror("nfct_labelmap_new");
>  }
> @@ -3212,6 +3220,10 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  			socketbuffersize = atol(optarg);
>  			options |= CT_OPT_BUFFERSIZE;
>  			break;
> +		case 'M':
> +			labelmap_path = strdup(optarg);

Should this exit() if labelmap_path != NULL?

> +	if (labelmap_path)
> +		free(labelmap_path);

free(NULL) is ok, so no need for the conditional.

Patch looks fine, but I think it would be good to have a preparation
patch that moves all labelmap_init() calls from do_parse() to into
do_command_ct().

Then the option ordering would not matter anymore.