On 18 Dec 2024, at 20:12, Frode Nordahl wrote:
> In preparation for adding support for parsing routes with multiple
> next hops per destination, we need dynamic storage for them.
>
> Storage for the primary next hop is set aside in struct route_data
> to avoid unnecessary memory allocations for the common case of one
> next hop.
>
> The ovs-router module does not support storing multiple next hop
> information, and the existing parser does not understand them, so
> we retain this behavior by only passing single next hop routes for
> storage in ovs-router.
>
> This patch makes this change in isolation so that it is easier to
> review subsequent patches.
>
> Signed-off-by: Frode Nordahl <[email protected]>
See some comments below.
//Eelco
> ---
> lib/route-table.c | 94 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 81 insertions(+), 13 deletions(-)
>
> diff --git a/lib/route-table.c b/lib/route-table.c
> index 4e8f35199..b6f0223c0 100644
> --- a/lib/route-table.c
> +++ b/lib/route-table.c
> @@ -32,6 +32,7 @@
> #include "netlink.h"
> #include "netlink-notifier.h"
> #include "netlink-socket.h"
> +#include "openvswitch/list.h"
> #include "openvswitch/ofpbuf.h"
> #include "ovs-router.h"
> #include "packets.h"
> @@ -47,7 +48,34 @@ VLOG_DEFINE_THIS_MODULE(route_table);
>
> COVERAGE_DEFINE(route_table_dump);
>
> +struct route_data_nexthop {
> + struct ovs_list nexthop_node;
> +
> + struct in6_addr addr;
> + char ifname[IFNAMSIZ]; /* Interface name. */
> +};
> +
> struct route_data {
> + /* Routes can have multiple next hops per destination.
> + *
> + * Each next hop has its own set of attributes such as address family,
> + * interface and IP address.
> + *
> + * When retrieving information about a route from the kernel, in the case
> + * of multiple next hops, information is provided as nested attributes.
> + *
> + * A linked list with struct route_data_nexthop entries is used to store
> + * this information as we parse each attribute.
> + *
> + * For the common case of one next hop, the nexthops list will contain a
> + * single entry pointing to the struct route_data _primary_next_hop
> + * element.
> + *
> + * Any dynamically allocated list elements can be freed with a call to
> the
Maybe this should be more strict, i.e. 'MUST be freed with a ....'
> + * route_data_destroy function. */
> + struct ovs_list nexthops;
> + struct route_data_nexthop _primary_next_hop;
> +
> /* Copied from struct rtmsg. */
> unsigned char rtm_dst_len;
> unsigned char rtm_protocol;
> @@ -56,8 +84,6 @@ struct route_data {
> /* Extracted from Netlink attributes. */
> struct in6_addr rta_dst; /* 0 if missing. */
> struct in6_addr rta_prefsrc; /* 0 if missing. */
> - struct in6_addr rta_gw;
> - char ifname[IFNAMSIZ]; /* Interface name. */
> uint32_t mark;
> uint32_t rta_table_id; /* 0 if missing. */
> uint32_t rta_priority; /* 0 if missing. */
> @@ -89,12 +115,24 @@ static bool route_table_valid = false;
> static void route_table_reset(void);
> static void route_table_handle_msg(const struct route_table_msg *);
> static int route_table_parse(struct ofpbuf *, void *change);
> -static void route_table_change(const struct route_table_msg *, void *);
> +static void route_table_change(struct route_table_msg *, void *aux);
> static void route_map_clear(void);
>
> static void name_table_init(void);
> static void name_table_change(const struct rtnetlink_change *, void *);
>
> +static void
> +route_data_destroy(struct route_data *rd)
> +{
> + struct route_data_nexthop *rdnh;
> +
> + LIST_FOR_EACH_POP (rdnh, nexthop_node, &rd->nexthops) {
> + if (rdnh && rdnh != &rd->_primary_next_hop) {
> + free(rdnh);
> + }
> + }
> +}
> +
> uint64_t
> route_table_get_change_seq(void)
> {
> @@ -190,6 +228,7 @@ route_table_dump_one_table(unsigned char id)
> filtered = false;
> }
> route_table_handle_msg(&msg);
> + route_data_destroy(&msg.rd);
> }
> }
> ofpbuf_uninit(&buf);
> @@ -222,13 +261,12 @@ route_table_reset(void)
> }
> }
>
> -/* Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, 0 on parse
> - * error. */
> static int
> route_table_parse__(struct ofpbuf *buf, size_t ofs,
> const struct nlmsghdr *nlmsg,
> const struct rtmsg *rtm, struct route_table_msg *change)
> {
> + struct route_data_nexthop *rdnh = NULL;
This can be defined lower in the if statement and moved in later patches if
required.
> bool parsed, ipv4 = false;
>
> static const struct nl_policy policy[] = {
> @@ -269,6 +307,12 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> int rta_oif; /* Output interface index. */
>
> memset(change, 0, sizeof *change);
> +
> + /* ovs_list_init / ovs_list_insert does not allocate any memory */
Don't think this comment is necessary. If you do think it is, change it to
something like:
/* ovs_list_init() and ovs_list_insert() do not allocate any memory. */
> + ovs_list_init(&change->rd.nexthops);
> + rdnh = &change->rd._primary_next_hop;
> + ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node);
> +
> change->relevant = true;
>
> if (rtm->rtm_scope == RT_SCOPE_NOWHERE) {
> @@ -292,7 +336,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> if (attrs[RTA_OIF]) {
> rta_oif = nl_attr_get_u32(attrs[RTA_OIF]);
>
> - if (!if_indextoname(rta_oif, change->rd.ifname)) {
> + if (!if_indextoname(rta_oif, rdnh->ifname)) {
> int error = errno;
>
> VLOG_DBG_RL(&rl, "Could not find interface name[%u]: %s",
> @@ -300,7 +344,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> if (error == ENXIO) {
> change->relevant = false;
Now that this becomes an external API, the content might still be used even if
relevant was set to false. So maybe it will be good to initalize the string in
the error case.
rdnh->ifname[0] = 0;
> } else {
> - return 0;
> + goto error_out;
> }
> }
> }
> @@ -330,9 +374,9 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> if (ipv4) {
> ovs_be32 gw;
> gw = nl_attr_get_be32(attrs[RTA_GATEWAY]);
> - in6_addr_set_mapped_ipv4(&change->rd.rta_gw, gw);
> + in6_addr_set_mapped_ipv4(&rdnh->addr, gw);
> } else {
> - change->rd.rta_gw = nl_attr_get_in6_addr(attrs[RTA_GATEWAY]);
> + rdnh->addr = nl_attr_get_in6_addr(attrs[RTA_GATEWAY]);
> }
> }
> if (attrs[RTA_MARK]) {
> @@ -343,13 +387,27 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> }
> } else {
> VLOG_DBG_RL(&rl, "received unparseable rtnetlink route message");
> - return 0;
> + goto error_out;
> }
>
> /* Success. */
> return ipv4 ? RTNLGRP_IPV4_ROUTE : RTNLGRP_IPV6_ROUTE;
> +
> +error_out:
> + route_data_destroy(&change->rd);
> + return 0;
> }
>
> +/* Parse Netlink message in buf, which is expected to contain UAPI rtmsg
> + * header and associated route attributes.
> + *
> + * Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, 0 on parse
> + * error.
> + *
> + * On success, memory may have be allocated, and it is the callers
> + * responsibility to free it with a call to route_data_destroy.
> + *
> + * In case of error, any allocated memory will be freed before return. */
Some spelling corrections, highlighted with **
/* Parse Netlink message in buf, which is expected to contain **a** UAPI rtmsg
* header and associated route attributes.
*
* Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, **and** 0 on
**a** parse
* error.
*
* On success, memory may have **been** allocated, and it is the **caller’s**
* responsibility to free it with a call to route_data_destroy**()**.
*
* In case of error, any allocated memory will be freed before **returning**. */
> static int
> route_table_parse(struct ofpbuf *buf, void *change)
> {
> @@ -373,7 +431,7 @@ route_table_standard_table(uint32_t table_id)
> }
>
> static void
> -route_table_change(const struct route_table_msg *change OVS_UNUSED,
> +route_table_change(struct route_table_msg *change,
> void *aux OVS_UNUSED)
Same comment as before; This will now fit on one line.
> {
> if (!change
> @@ -381,16 +439,26 @@ route_table_change(const struct route_table_msg *change
> OVS_UNUSED,
> && route_table_standard_table(change->rd.rta_table_id))) {
> route_table_valid = false;
> }
> + if (change) {
> + route_data_destroy(&change->rd);
> + }
> }
>
> static void
> route_table_handle_msg(const struct route_table_msg *change)
> {
> - if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) {
> + if (change->relevant && change->nlmsg_type == RTM_NEWROUTE
> + && ovs_list_is_singleton(&change->rd.nexthops)) {
Based on your own comment, I would change it here to
!ovs_list_is_empty(&change->rd.nexthops) in this patch already.
> const struct route_data *rd = &change->rd;
> + const struct route_data_nexthop *rdnh;
> +
> + /* The ovs-router module does currently not implement lookup nor
> + * storage for routes with multiple next hops. */
Maybe enhance the comment with:
/* The ovs-router module currently does not implement lookup or
* storage for routes with multiple next hops. For backwards
* compatibility, we use the first next hop. */
> + rdnh = CONTAINER_OF(ovs_list_front(&change->rd.nexthops),
> + const struct route_data_nexthop, nexthop_node);
>
> ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len,
> - rd->local, rd->ifname, &rd->rta_gw,
> + rd->local, rdnh->ifname, &rdnh->addr,
> &rd->rta_prefsrc);
> }
> }
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
