On Mon, Oct 4, 2021 at 11:41 PM Nicolas Dichtel <[email protected]> wrote: > > Le 01/10/2021 à 22:42, Cpp Code a écrit : > > On Fri, Oct 1, 2021 at 12:21 AM Nicolas Dichtel > > <[email protected]> wrote: > >> > >> Le 30/09/2021 à 18:11, Cpp Code a écrit : > >>> On Wed, Sep 29, 2021 at 6:19 AM Jakub Kicinski <[email protected]> wrote: > >>>> > >>>> On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote: > >>>>>> /* Insert a kernel only KEY_ATTR */ > >>>>>> #define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX > >>>>>> #undef OVS_KEY_ATTR_MAX > >>>>>> #define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX > >>>>> Following the other thread [1], this will break if a new app runs over > >>>>> an old > >>>>> kernel. > >>>> > >>>> Good point. > >>>> > >>>>> Why not simply expose this attribute to userspace and throw an error if > >>>>> a > >>>>> userspace app uses it? > >>>> > >>>> Does it matter if it's exposed or not? Either way the parsing policy > >>>> for attrs coming from user space should have a reject for the value. > >>>> (I say that not having looked at the code, so maybe I shouldn't...) > >>> > >>> To remove some confusion, there are some architectural nuances if we > >>> want to extend code without large refactor. > >>> The ovs_key_attr is defined only in kernel side. Userspace side is > >>> generated from this file. As well the code can be built without kernel > >>> modules. > >>> The code inside OVS repository and net-next is not identical, but I > >>> try to keep some consistency. > >> I didn't get why OVS_KEY_ATTR_TUNNEL_INFO cannot be exposed to userspace. > > > > OVS_KEY_ATTR_TUNNEL_INFO is compressed version of OVS_KEY_ATTR_TUNNEL > > and for clarity purposes its not exposed to userspace as it will never > > use it. > > I would say it's a coding style as it would not brake anything if exposed. > In fact, it's the best way to keep the compatibility in the long term. > You can define it like this: > OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info, reserved for kernel use > */ > > > > >> > >>> > >>> JFYI This is the file responsible for generating userspace part: > >>> https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h > >>> This is the how corresponding file for ovs_key_attr looks inside OVS: > >>> https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h > >>> one can see there are more values than in net-next version. > >> There are still some '#ifdef __KERNEL__'. The standard 'make > >> headers_install' > >> filters them. Why not using this standard mechanism? > > > > Could you elaborate on this, I don't quite understand the idea!? Which > > ifdef you are referring, the one along OVS_KEY_ATTR_TUNNEL_INFO or > > some other? > My understanding is that this file is used for the userland third party, thus, > theoretically, there should be no '#ifdef __KERNEL__'. uapi headers generated > with 'make headers_install' are filtered to remove them.
From https://lwn.net/Articles/507794/ I understand that is the goal, but this part of the code is still used in the kernel part. > > > > >> > >> In this file, there are two attributes (OVS_KEY_ATTR_PACKET_TYPE and > >> OVS_KEY_ATTR_ND_EXTENSIONS) that doesn't exist in the kernel. > >> This will also breaks if an old app runs over a new kernel. I don't see > >> how it > >> is possible to keep the compat between {old|new} {kernel|app}. > > > > Looks like this most likely is a bug while working on multiple > > versions of code. Need to do add more padding. > As said above, just define the same uapi for everybody and the problem is gone > forever. > As this part of the code was already there, I think the correct way would be to refactor that in a separate commit if necessary. > > Regards, > Nicolas Best, Tom _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
