On Thu, Jun 26, 2025 at 02:52:48AM +0200, Florian Westphal wrote:
> Its not enough to check for interval flag, this would assert in interval
> code due to concat being passed to the interval code:
> BUG: unhandled key type 13
>
> After fix:
> same_set_name_but_different_keys_assert:8:6-7: Error: set already exists with
> different datatype (concatenation of (IPv4 address, network interface index) vs
> network interface index)
> set s4 {
> ^^
>
> This also improves error verbosity when mixing datamap and objref maps:
>
> invalid_transcation_merge_map_and_objref_map:9:13-13:
> Error: map already exists with different datatype (IPv4 address vs string)
>
> .. instead of 'Cannot merge map with incompatible existing map of same name'.
> The 'Cannot merge map with incompatible existing map of same name' check
> is kept in place to catch when ruleset contains a set and map with same name
> and same key definition.
LGTM, thanks.
> Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
Reviewed-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> Followup to previous
> '[nft,v2] evaluate: check that set type is identical before merging',
> it has the welcome side effect to improve error reporting as well.
>
> src/evaluate.c | 12 ++++++++++++
> src/intervals.c | 2 +-
> .../nft-f/same_set_name_but_different_keys_assert | 13 +++++++++++++
> 3 files changed, 26 insertions(+), 1 deletion(-)
> create mode 100644 tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert
>
> diff --git a/src/evaluate.c b/src/evaluate.c
> index fc9d82f73b68..a2d5d7c29514 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -5304,6 +5304,18 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
> if (set_is_interval(set->flags) && !set_is_interval(existing_set->flags))
> return set_error(ctx, set,
> "existing %s lacks interval flag", type);
> + if (set->data && existing_set->data &&
> + !datatype_equal(existing_set->data->dtype, set->data->dtype))
> + return set_error(ctx, set,
> + "%s already exists with different datatype (%s vs %s)",
> + type, existing_set->data->dtype->desc,
> + set->data->dtype->desc);
> + if (!datatype_equal(existing_set->key->dtype, set->key->dtype))
> + return set_error(ctx, set,
> + "%s already exists with different datatype (%s vs %s)",
> + type, existing_set->key->dtype->desc,
> + set->key->dtype->desc);
> + /* Catch attempt to merge set and map */
> if (!set_type_compatible(set, existing_set))
> return set_error(ctx, set, "Cannot merge %s with incompatible existing %s of same name",
> type,
> diff --git a/src/intervals.c b/src/intervals.c
> index bf125a0c59d3..e5bbb0384964 100644
> --- a/src/intervals.c
> +++ b/src/intervals.c
> @@ -70,7 +70,7 @@ static void setelem_expr_to_range(struct expr *expr)
> expr->key = key;
> break;
> default:
> - BUG("unhandled key type %d\n", expr->key->etype);
> + BUG("unhandled key type %s\n", expr_name(expr->key));
> }
> }
>
> diff --git a/tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert b/tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert
> new file mode 100644
> index 000000000000..8fcfdf5cba03
> --- /dev/null
> +++ b/tests/shell/testcases/bogons/nft-f/same_set_name_but_different_keys_assert
> @@ -0,0 +1,13 @@
> +table ip t {
> + set s4 {
> + type ipv4_addr . iface_index
> + flags interval
> + elements = { 127.0.0.1 . "lo" }
> + }
> +
> + set s4 {
> + type iface_index
> + flags interval
> + elements = { "lo" }
> + }
> +}
> --
> 2.49.0
>
>
