Thanks for the review!
On Fri Jun 13, 2025 at 2:18 PM CEST, Florian Westphal wrote:
> Christoph Heiss <c.heiss@xxxxxxxxxxx> 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/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?
Honestly, the option parsing is quite convoluted.
But it's used for indexing into `optflags`, which in turned is used by
generic_opt_check().
As `--labelmap` can only be used with `--label{-add,-del}` and thus
`-L`, `-E`, `-U` and `-D`, this is appropriately reflected in
`commands_v_options`.
Based on that, generic_opt_check() will then throw an error/abort if
`--labelmap` is used with any other command, e.g.:
conntrack v1.4.8 (conntrack-tools): Illegal option `--labelmap' with this command
Try `conntrack -h' or 'conntrack --help' for more information.
At least what I got after tracing quite a bit through the code.
>> [..]
>> @@ -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?
Don't have a strong preference on this, but probably makes sense to
abort in case the user ever specifies the option multiple times. I'll
add that.
>
>> + if (labelmap_path)
>> + free(labelmap_path);
>
> free(NULL) is ok, so no need for the conditional.
Ack.
>
> 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.
Can do that. Will just entail a bit more refactoring around the
--label{-add,-del} option parsing, as that relies on an already
initialized labelmap.
