On Fri, Feb 28, 2025 at 08:21:01PM +0100, Ilya Maximets wrote: > On 2/27/25 13:51, Felix Huettner wrote: > > On Thu, Feb 27, 2025 at 11:37:17AM +0100, Ilya Maximets wrote: > >> On 2/27/25 10:47, Felix Huettner via dev wrote: > >>> RTPROT_OVN has been merged to the net-next tree of the kernel just now > >>> [1]. > >>> Until it is available on all systems we need to forward declare it. > >>> > >>> As we are already on it we also add support for it in > >>> test-lib-route-table. > >>> > >>> [1]: > >>> https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=6002850fdfe0b4343136670a9895b6ba4ee3285b > >>> > >>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud> > >>> --- > >>> acinclude.m4 | 6 ++++++ > >>> include/linux/automake.mk | 1 + > >>> include/linux/rtnetlink.h | 12 ++++++++++++ > >>> tests/test-lib-route-table.c | 1 + > >>> 4 files changed, 20 insertions(+) > >>> create mode 100644 include/linux/rtnetlink.h > >>> > >>> diff --git a/acinclude.m4 b/acinclude.m4 > >>> index 8658bcfcb..278bad583 100644 > >>> --- a/acinclude.m4 > >>> +++ b/acinclude.m4 > >>> @@ -155,6 +155,12 @@ AC_DEFUN([OVS_CHECK_LINUX_NETLINK], [ > >>> ])], > >>> [AC_DEFINE([HAVE_NLA_BITFIELD32], [1], > >>> [Define to 1 if struct nla_bitfield32 is available.])]) > >>> + AC_COMPILE_IFELSE([ > >>> + AC_LANG_PROGRAM([#include <linux/rtnetlink.h>], [ > >>> + int x = RTPROT_OVN; > >>> + ])], > >>> + [AC_DEFINE([HAVE_RTPROT_OVN], [1], > >>> + [Define to 1 if RTPROT_OVN is available.])]) > >>> ]) > >>> > >>> dnl OVS_CHECK_LINUX_TC > >>> diff --git a/include/linux/automake.mk b/include/linux/automake.mk > >>> index ac306b53c..4a92d45ef 100644 > >>> --- a/include/linux/automake.mk > >>> +++ b/include/linux/automake.mk > >>> @@ -3,6 +3,7 @@ noinst_HEADERS += \ > >>> include/linux/netfilter/nf_conntrack_sctp.h \ > >>> include/linux/openvswitch.h \ > >>> include/linux/pkt_cls.h \ > >>> + include/linux/rtnetlink.h \ > >>> include/linux/psample.h \ > >>> include/linux/gen_stats.h \ > >>> include/linux/tc_act/tc_mpls.h \ > >>> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h > >>> new file mode 100644 > >>> index 000000000..deac39677 > >>> --- /dev/null > >>> +++ b/include/linux/rtnetlink.h > >>> @@ -0,0 +1,12 @@ > >>> +#ifndef __UAPI_LINUX_RTNETLINK_WRAPPER_H > >>> +#define __UAPI_LINUX_RTNETLINK_WRAPPER_H 1 > >>> + > >>> +#if !defined(__KERNEL__) && !defined(HAVE_RTPROT_OVN) > >>> + > >>> +#define RTPROT_OVN 84 > >>> + > >>> +#endif /* !__KERNEL__ && !HAVE_RTPROT_OVN */ > >>> + > >>> +#include_next <linux/rtnetlink.h> > >>> + > >>> +#endif /* __UAPI_LINUX_RTNETLINK_WRAPPER_H */ > >> > >> Hi, Felix. Thanks for the patch! > >> > >> It seems a little excessive to create the whole file and a configuration > >> time check just for one definition. We normally just ifndef+define this > >> kind of things. See some examples at the top of lib/netdev-linux.c or > >> lib/dpif-netlink-rtnl.c. > >> > >> Same, probably, applies to the OVN patch. > > > > Hi Ilya, thanks for the feedback. > > > > The idea to have this similar to include/linux/netlink.h came from > > https://mail.openvswitch.org/pipermail/ovs-dev/2025-February/421511.html
Hi Ilya, > > The difference with netlink.h is that we're defining the missing struct > in there. It's hard to simply ifndef the structure definition, we have > to detect it at configuration time. A simple macro on the other hand > is simple to detect with a basic ifndef. There is no point in checking > it at the configuration time, as we'll just be defining a new macro based > on this one being defined or not and that doesn't make a lot of sense. ok, i'll change it to an ifndef in the next version. > > > > > I would also be fine if we would add the ifndef/define to the individual > > source files. I am now not sure what is the preferred way. > > Just ifndef it, IMO. :) > > > > > Having this in ovs to then just "#include <linux/rtnetlink.h>" is > > definately convenient though :) > > Maybe, but it's also not that hard to define this macro once. If we need > to define it multiple times, then the ifndef can be done in some internal > header to avoid duplication. sounds good.j > > Also, I'm not really sure if the build assertion is needed on the OVN side. > Kernel uAPI supposed to never change, so the number should actually never > change. And the tests will fail if it does, that will be caught. But > no strong opinion on that one. +1 Thanks a lot for the review, Felix > > Best regards, Ilya Maximets. > > > > > Thanks a lot, > > Felix > > > >> > >> Best regards, Ilya Maximets. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
