| Message ID | 070caa7d1c655e3603e8a7f258a868b4bd22aa77.1732630344.git.felix.huettner@stackit.cloud |
|---|---|
| State | Changes Requested |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | OVS Patches for OVN Fabric Integration | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
On Tue, Nov 26, 2024 at 3:37 PM Felix Huettner via dev <ovs-dev@openvswitch.org> wrote: > > Previously the data generated in route-table was not exposed to OVN. > This is now changed and we expose a few additional fields we get from > the kernel. The subject and commit message of this patch is taking a bit too narrow a view of the world. While it is true that we want to use this in OVN, the Open vSwitch library has many users. This is a base library and you need to think more broadly about this and put it in a generic context. As discussed in previous patches please also separate these changes from the introduction of the namespace feature. Most of this appears to come straight out of https://mail.openvswitch.org/pipermail/ovs-dev/2024-July/416042.html too, so perhaps some attribution is in order? > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud> > --- > lib/route-table.c | 52 +++++++++++++++++++++-------------------------- > lib/route-table.h | 31 ++++++++++++++++++++++++++++ > 2 files changed, 54 insertions(+), 29 deletions(-) > > diff --git a/lib/route-table.c b/lib/route-table.c > index 270ad489a..d7d86f9ae 100644 > --- a/lib/route-table.c > +++ b/lib/route-table.c > @@ -47,27 +47,6 @@ VLOG_DEFINE_THIS_MODULE(route_table); > > COVERAGE_DEFINE(route_table_dump); > > -struct route_data { > - /* Copied from struct rtmsg. */ > - unsigned char rtm_dst_len; > - bool local; > - > - /* 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; > -}; > - > -/* A digested version of a route message sent down by the kernel to indicate > - * that a route has changed. */ > -struct route_table_msg { > - bool relevant; /* Should this message be processed? */ > - int nlmsg_type; /* e.g. RTM_NEWROUTE, RTM_DELROUTE. */ > - struct route_data rd; /* Data parsed from this message. */ > -}; > - > static struct ovs_mutex route_table_mutex = OVS_MUTEX_INITIALIZER; > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); > > @@ -84,7 +63,8 @@ static struct nln_notifier *name_notifier = NULL; > static bool route_table_valid = false; > > static void route_table_reset(void); > -static void route_table_handle_msg(const struct route_table_msg *); > +static void route_table_handle_msg(const struct route_table_msg *, > + void *); According to the subject, this patch is about exposing existing functionality so that it can be consumed outside of the route-table module. This line is a different type of change, and you need to put it in a separate commit with subject and commit message explaining the change. I see there are many similar issues below, so I stopped reviewing this patch here. > static int route_table_parse_ns(struct ofpbuf *, void *change, > const char *netns); > static void route_table_change(const struct route_table_msg *, void *); > @@ -156,8 +136,9 @@ route_table_wait(void) > ovs_mutex_unlock(&route_table_mutex); > } > > -static bool > -route_table_dump_one_table(const char *netns, unsigned char id) > +bool > +route_table_dump_one_table(char *netns, uint32_t id, > + route_table_dump_callback_t handle_msg, void *data) > { > uint64_t reply_stub[NL_DUMP_BUFSIZE / 8]; > struct ofpbuf request, reply, buf; > @@ -171,7 +152,9 @@ route_table_dump_one_table(const char *netns, unsigned char id) > > rq_msg = ofpbuf_put_zeros(&request, sizeof *rq_msg); > rq_msg->rtm_family = AF_UNSPEC; > - rq_msg->rtm_table = id; > + rq_msg->rtm_table = RT_TABLE_UNSPEC; > + > + nl_msg_put_u32(&request, RTA_TABLE, id); > nl_ns_dump_start(netns, &dump, NETLINK_ROUTE, &request); > ofpbuf_uninit(&request); > @@ -187,7 +170,7 @@ route_table_dump_one_table(const char *netns, unsigned char id) > if (!(nlmsghdr->nlmsg_flags & NLM_F_DUMP_FILTERED)) { > filtered = false; > } > - route_table_handle_msg(&msg); > + (*handle_msg)(&msg, data); > } > } > ofpbuf_uninit(&buf); > @@ -213,7 +196,9 @@ route_table_reset(void) > COVERAGE_INC(route_table_dump); > > for (size_t i = 0; i < ARRAY_SIZE(tables); i++) { > - if (!route_table_dump_one_table(NULL, tables[i])) { > + if (!route_table_dump_one_table(NULL, tables[i], > + route_table_handle_msg, > + NULL)) { > /* Got unfiltered reply, no need to dump further. */ > break; > } > @@ -236,6 +221,7 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, > [RTA_MARK] = { .type = NL_A_U32, .optional = true }, > [RTA_PREFSRC] = { .type = NL_A_U32, .optional = true }, > [RTA_TABLE] = { .type = NL_A_U32, .optional = true }, > + [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, > }; > > static const struct nl_policy policy6[] = { > @@ -245,6 +231,7 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, > [RTA_GATEWAY] = { .type = NL_A_IPV6, .optional = true }, > [RTA_PREFSRC] = { .type = NL_A_IPV6, .optional = true }, > [RTA_TABLE] = { .type = NL_A_U32, .optional = true }, > + [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, > }; > > struct nlattr *attrs[ARRAY_SIZE(policy)]; > @@ -293,11 +280,14 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, > && table_id != RT_TABLE_MAIN > && table_id != RT_TABLE_LOCAL) { > change->relevant = false; > - } > + } > + change->rd.rta_table_id = table_id; > > change->nlmsg_type = nlmsg->nlmsg_type; > change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0); > change->rd.local = rtm->rtm_type == RTN_LOCAL; > + change->rd.plen = rtm->rtm_dst_len; > + change->rd.rtm_protocol = rtm->rtm_protocol; > if (attrs[RTA_OIF]) { > rta_oif = nl_attr_get_u32(attrs[RTA_OIF]); > > @@ -347,6 +337,9 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, > if (attrs[RTA_MARK]) { > change->rd.mark = nl_attr_get_u32(attrs[RTA_MARK]); > } > + if (attrs[RTA_PRIORITY]) { > + change->rd.rta_priority = nl_attr_get_u32(attrs[RTA_PRIORITY]); > + } > } else { > VLOG_DBG_RL(&rl, "received unparseable rtnetlink route message"); > return 0; > @@ -366,7 +359,8 @@ route_table_change(const struct route_table_msg *change OVS_UNUSED, > } > > static void > -route_table_handle_msg(const struct route_table_msg *change) > +route_table_handle_msg(const struct route_table_msg *change, > + void *data OVS_UNUSED) > { > if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) { > const struct route_data *rd = &change->rd; > diff --git a/lib/route-table.h b/lib/route-table.h > index 3a02d737a..b3d2a8741 100644 > --- a/lib/route-table.h > +++ b/lib/route-table.h > @@ -26,6 +26,31 @@ > > #include "openvswitch/types.h" > > +struct route_data { > + /* Copied from struct rtmsg. */ > + unsigned char rtm_dst_len; > + bool local; > + > + /* 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. */ > + unsigned char plen; > + unsigned char rtm_protocol; > + uint32_t rta_priority; > +}; Again, put the exposure of existing functions and change of behavior in separate commits. It becomes particularly evident why you need to do that here, as the above addition is quite different from the subtraction of the similar struct. Moving and changing things around simultaneously makes it impossible to review. -- Frode Nordahl > +/* A digested version of a route message sent down by the kernel to indicate > + * that a route has changed. */ > +struct route_table_msg { > + bool relevant; /* Should this message be processed? */ > + int nlmsg_type; /* e.g. RTM_NEWROUTE, RTM_DELROUTE. */ > + struct route_data rd; /* Data parsed from this message. */ > +}; > + > uint64_t route_table_get_change_seq(void); > void route_table_init(void); > void route_table_run(void); > @@ -33,4 +58,10 @@ void route_table_wait(void); > bool route_table_fallback_lookup(const struct in6_addr *ip6_dst, > char name[], > struct in6_addr *gw6); > + > +typedef void (route_table_dump_callback_t)(const struct route_table_msg *msg, > + void *data); > +bool route_table_dump_one_table(char *netns, uint32_t id, > + route_table_dump_callback_t handle_msg, > + void *data); > #endif /* route-table.h */ > -- > 2.47.0 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/route-table.c b/lib/route-table.c index 270ad489a..d7d86f9ae 100644 --- a/lib/route-table.c +++ b/lib/route-table.c @@ -47,27 +47,6 @@ VLOG_DEFINE_THIS_MODULE(route_table); COVERAGE_DEFINE(route_table_dump); -struct route_data { - /* Copied from struct rtmsg. */ - unsigned char rtm_dst_len; - bool local; - - /* 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; -}; - -/* A digested version of a route message sent down by the kernel to indicate - * that a route has changed. */ -struct route_table_msg { - bool relevant; /* Should this message be processed? */ - int nlmsg_type; /* e.g. RTM_NEWROUTE, RTM_DELROUTE. */ - struct route_data rd; /* Data parsed from this message. */ -}; - static struct ovs_mutex route_table_mutex = OVS_MUTEX_INITIALIZER; static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); @@ -84,7 +63,8 @@ static struct nln_notifier *name_notifier = NULL; static bool route_table_valid = false; static void route_table_reset(void); -static void route_table_handle_msg(const struct route_table_msg *); +static void route_table_handle_msg(const struct route_table_msg *, + void *); static int route_table_parse_ns(struct ofpbuf *, void *change, const char *netns); static void route_table_change(const struct route_table_msg *, void *); @@ -156,8 +136,9 @@ route_table_wait(void) ovs_mutex_unlock(&route_table_mutex); } -static bool -route_table_dump_one_table(const char *netns, unsigned char id) +bool +route_table_dump_one_table(char *netns, uint32_t id, + route_table_dump_callback_t handle_msg, void *data) { uint64_t reply_stub[NL_DUMP_BUFSIZE / 8]; struct ofpbuf request, reply, buf; @@ -171,7 +152,9 @@ route_table_dump_one_table(const char *netns, unsigned char id) rq_msg = ofpbuf_put_zeros(&request, sizeof *rq_msg); rq_msg->rtm_family = AF_UNSPEC; - rq_msg->rtm_table = id; + rq_msg->rtm_table = RT_TABLE_UNSPEC; + + nl_msg_put_u32(&request, RTA_TABLE, id); nl_ns_dump_start(netns, &dump, NETLINK_ROUTE, &request); ofpbuf_uninit(&request); @@ -187,7 +170,7 @@ route_table_dump_one_table(const char *netns, unsigned char id) if (!(nlmsghdr->nlmsg_flags & NLM_F_DUMP_FILTERED)) { filtered = false; } - route_table_handle_msg(&msg); + (*handle_msg)(&msg, data); } } ofpbuf_uninit(&buf); @@ -213,7 +196,9 @@ route_table_reset(void) COVERAGE_INC(route_table_dump); for (size_t i = 0; i < ARRAY_SIZE(tables); i++) { - if (!route_table_dump_one_table(NULL, tables[i])) { + if (!route_table_dump_one_table(NULL, tables[i], + route_table_handle_msg, + NULL)) { /* Got unfiltered reply, no need to dump further. */ break; } @@ -236,6 +221,7 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, [RTA_MARK] = { .type = NL_A_U32, .optional = true }, [RTA_PREFSRC] = { .type = NL_A_U32, .optional = true }, [RTA_TABLE] = { .type = NL_A_U32, .optional = true }, + [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, }; static const struct nl_policy policy6[] = { @@ -245,6 +231,7 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, [RTA_GATEWAY] = { .type = NL_A_IPV6, .optional = true }, [RTA_PREFSRC] = { .type = NL_A_IPV6, .optional = true }, [RTA_TABLE] = { .type = NL_A_U32, .optional = true }, + [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, }; struct nlattr *attrs[ARRAY_SIZE(policy)]; @@ -293,11 +280,14 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, && table_id != RT_TABLE_MAIN && table_id != RT_TABLE_LOCAL) { change->relevant = false; - } + } + change->rd.rta_table_id = table_id; change->nlmsg_type = nlmsg->nlmsg_type; change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0); change->rd.local = rtm->rtm_type == RTN_LOCAL; + change->rd.plen = rtm->rtm_dst_len; + change->rd.rtm_protocol = rtm->rtm_protocol; if (attrs[RTA_OIF]) { rta_oif = nl_attr_get_u32(attrs[RTA_OIF]); @@ -347,6 +337,9 @@ route_table_parse_ns(struct ofpbuf *buf, void *change_, if (attrs[RTA_MARK]) { change->rd.mark = nl_attr_get_u32(attrs[RTA_MARK]); } + if (attrs[RTA_PRIORITY]) { + change->rd.rta_priority = nl_attr_get_u32(attrs[RTA_PRIORITY]); + } } else { VLOG_DBG_RL(&rl, "received unparseable rtnetlink route message"); return 0; @@ -366,7 +359,8 @@ route_table_change(const struct route_table_msg *change OVS_UNUSED, } static void -route_table_handle_msg(const struct route_table_msg *change) +route_table_handle_msg(const struct route_table_msg *change, + void *data OVS_UNUSED) { if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) { const struct route_data *rd = &change->rd; diff --git a/lib/route-table.h b/lib/route-table.h index 3a02d737a..b3d2a8741 100644 --- a/lib/route-table.h +++ b/lib/route-table.h @@ -26,6 +26,31 @@ #include "openvswitch/types.h" +struct route_data { + /* Copied from struct rtmsg. */ + unsigned char rtm_dst_len; + bool local; + + /* 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. */ + unsigned char plen; + unsigned char rtm_protocol; + uint32_t rta_priority; +}; + +/* A digested version of a route message sent down by the kernel to indicate + * that a route has changed. */ +struct route_table_msg { + bool relevant; /* Should this message be processed? */ + int nlmsg_type; /* e.g. RTM_NEWROUTE, RTM_DELROUTE. */ + struct route_data rd; /* Data parsed from this message. */ +}; + uint64_t route_table_get_change_seq(void); void route_table_init(void); void route_table_run(void); @@ -33,4 +58,10 @@ void route_table_wait(void); bool route_table_fallback_lookup(const struct in6_addr *ip6_dst, char name[], struct in6_addr *gw6); + +typedef void (route_table_dump_callback_t)(const struct route_table_msg *msg, + void *data); +bool route_table_dump_one_table(char *netns, uint32_t id, + route_table_dump_callback_t handle_msg, + void *data); #endif /* route-table.h */
Previously the data generated in route-table was not exposed to OVN. This is now changed and we expose a few additional fields we get from the kernel. Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud> --- lib/route-table.c | 52 +++++++++++++++++++++-------------------------- lib/route-table.h | 31 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 29 deletions(-)