On Mon, Oct 19, 2026 at 2:25 PM Srikar Tati via dev
<[email protected]> wrote:
>
> From: Srikar Tati <[email protected]>
>
> Static routes choose the fields that their ECMP group hashes on with
> the "selection_fields" column. The following patches let learned
> routes and EVPN FDB entries take the same fields from a string option.
>
> Add ovn_ecmp_selection_fields_parse() to parse such an option with the
> fields that "selection_fields" accepts, and move the static route
> handling of tp_src and tp_dst to ovn_ecmp_selection_fields_add(), so
> that every user adds ip_proto in the same way.
>
> Assisted-by: Claude Opus 5.5, Claude Code
> Signed-off-by: Srikar Tati <[email protected]>
Hi Srikar,
Thanks for the patches. A few minor comments below.
Thanks
Numan
> ---
> lib/ovn-util.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
> lib/ovn-util.h | 5 +++++
> northd/northd.c | 17 +++--------------
> 3 files changed, 59 insertions(+), 14 deletions(-)
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index eb1fa8a06..4ec648318 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -1634,6 +1634,57 @@ ovn_is_valid_vni(int64_t vni)
> return vni >= 0 && (vni <= (1 << 24) - 1);
> }
>
> +/* Adds 'field' to the ECMP 'selection_fields'. If 'field' is tp_src or
> + * tp_dst, also adds ip_proto: the protocol specific port fields are only
> + * valid with a match on the protocol. */
> +void
> +ovn_ecmp_selection_fields_add(struct sset *selection_fields,
> + const char *field)
Since 'selection_fields' is an output param, let's make it the last param.
i.e change the signature to
void
ovn_ecmp_selection_fields_add(const char *field, struct sset *selection_fields)
> +{
> + if (!strcmp(field, "tp_src") || !strcmp(field, "tp_dst")) {
> + sset_add(selection_fields, "ip_proto");
> + }
> + sset_add(selection_fields, field);
> +}
> +
> +/* Parses 'fields_s', a comma separated list of the fields that the
> + * Logical_Router_Static_Route selection_fields column accepts, into
> + * 'selection_fields'. Returns false, and leaves 'selection_fields'
> + * empty, if 'fields_s' contains a field that is not supported. */
> +bool
> +ovn_ecmp_selection_fields_parse(const char *fields_s,
> + struct sset *selection_fields)
> +{
> + static const char *const supported[] = {
> + "eth_src", "eth_dst", "ip_proto", "ip_src", "ip_dst",
> + "ipv6_src", "ipv6_dst", "tp_src", "tp_dst",
> + };
> +
> + char *tokstr = xstrdup(fields_s);
> + char *save_ptr = NULL;
> + bool ok = true;
> + for (char *field = strtok_r(tokstr, ",", &save_ptr); field;
> + field = strtok_r(NULL, ",", &save_ptr)) {
> + size_t i;
> + for (i = 0; i < ARRAY_SIZE(supported); i++) {
> + if (!strcmp(field, supported[i])) {
> + break;
> + }
> + }
> + if (i == ARRAY_SIZE(supported)) {
> + ok = false;
> + break;
> + }
> + ovn_ecmp_selection_fields_add(selection_fields, field);
> + }
> + free(tokstr);
> +
> + if (!ok) {
> + sset_clear(selection_fields);
> + }
> + return ok;
> +}
> +
> struct sset *
> lrp_network_sset(const char **networks, int n_networks)
> {
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 01d21d282..fc88bf6d6 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -735,6 +735,11 @@ void ovn_debug_commands_register(void);
>
> bool ovn_is_valid_vni(int64_t vni);
>
> +void ovn_ecmp_selection_fields_add(struct sset *selection_fields,
> + const char *field);
> +bool ovn_ecmp_selection_fields_parse(const char *fields_s,
> + struct sset *selection_fields);
> +
> const struct sbrec_port_binding *lport_lookup_by_name(
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> const char *name);
> diff --git a/northd/northd.c b/northd/northd.c
> index 7c4cb5a62..824c90c89 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -13036,20 +13036,9 @@ parsed_routes_add_static(const struct ovn_datapath
> *od,
>
> struct sset ecmp_selection_fields =
> SSET_INITIALIZER(&ecmp_selection_fields);
> -
> - /* If tp_src or tp_dst is included in the selection_fields, implicitly
> - * include match on ip_proto. */
> - if (route->n_selection_fields) {
> - struct sset field_set = SSET_INITIALIZER(&field_set);
> - for (size_t i = 0; i < route->n_selection_fields; i++) {
> - char *field = route->selection_fields[i];
> - if (!strcmp(field, "tp_src") || !strcmp(field, "tp_dst")) {
> - sset_add(&field_set, "ip_proto");
> - }
> - sset_add(&field_set, field);
> - }
> - sset_clone(&ecmp_selection_fields, &field_set);
> - sset_destroy(&field_set);
> + for (size_t i = 0; i < route->n_selection_fields; i++) {
> + ovn_ecmp_selection_fields_add(&ecmp_selection_fields,
> + route->selection_fields[i]);
> }
>
> uint32_t route_table_id = get_route_table_id(route_tables,
> --
Just a small nit: I see that no one is using the util function
'ovn_ecmp_selection_fields_parse()' in this patch.
But it is used in the next patch. I'd suggest moving this function to
the next patch.
With my comments addressed:
Acked-by: Numan Siddique <[email protected]>
Numan
> 2.54.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
