[ovs-dev] [PATCH 2/3] Do not seemingly #include Linux-specific headers on other platforms.

[Jesse Gross]

On Thu, Jul 31, 2014 at 7:52 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Jun 23, 2014 at 03:39:32PM -0700, Jesse Gross wrote:
>> On Fri, Jun 13, 2014 at 3:28 PM, Ben Pfaff <blp at nicira.com> wrote:
>> > Until now, the OVS source tree has had a whole maze of header files that
>> > make "#include <linux/openvswitch.h>" work OK regardless of platform, but
>> > this confuses everyone new to the tree, at first glance, and is difficult
>> > to understand at second glance too.
>> >
>> > This commit moves the core of the Netlink definitions that were in
>> > include/linux/openvswitch.h into a new header
>> > include/odp-netlink-internal.h, and then adds two wrappers:
>> >
>> >     * datapath/linux/compat/include/linux/openvswitch.h: This wrapper
>> >       allows the Linux datapath code to continue using "#include
>> >       <linux/openvswitch.h>".
>> >
>> >     * lib/odp-netlink.h: Userspace code uses this wrapper.
>> >
>> > This change allows all of the include/linux/* files to be deleted.
>> >
>> > Signed-off-by: Ben Pfaff <blp at nicira.com>
>>
>> I'm somewhat worried that diverging our copy of linux/openvswitch.h
>> will make things harder to maintain, especially since we've tried to
>> reduce differences between upstream and the out-of-tree module as much
>> as possible.
>>
>> I was imagining that we would model this as if we were compiling
>> purely against the upstream kernel and openvswitch.h was an exported
>> header (since it is, we just happen to have a compatibility backport).
>> In that case, we wouldn't be able to change the original header and
>> presumably Linux-specific code would translate to something
>> OVS-internal.
>
> I've been starting to think about this again since (as you pointed out
> elsewhere) it came up in the hyperv port.
>
> I'm not too sure what you're envisioning in the model that you're
> thinking of.  Here are my thoughts on how to do this in terms of
> backports:
>
>         * We'd need some base version of the whole header file, because
>           not every build environment would have any version at all
>           (given that we build on non-Linux and often system Linux
>           header files are for kernels older than the one actually in
>           use).
>
>         * We'd need to detect the presence of numerous "enum" values and
>           define them if not present (or we could unconditionally
>           #define them as backports whether present or not, since using
>           #defines as overlays for enums is mostly harmless).
>
>         * We'd need to detect the presence of numerous structs and
>           define them if not present.
>
> It might be more work to maintain a set of backports than a slightly
> different header file.  (It's definitely more work than what we're doing
> now.)

When I said "backport", I didn't really mean that we need to take
things piece-by-piece. It was really more of a way to view things.

Let's pretend that all of the features from the out of tree kernel
module are available in the currently running kernel and the userspace
header is up to date, since this is our goal. In this case, we would
just use the installed header and running kernel. This is what we do
for all other non-OVS Linux features.

Now obviously this is not really the case because the OVS kernel
module is still relatively young and fast moving and therefore has not
reached the point we can safely assume that the features available in
the current kernel will satisfy the majority of users. For the kernel
code, this means that we have a backported module. For the interface,
we also have a backport currently (even if it happens to completely
replace the exported header file, I still consider this a backport -
we do a full replacement for some kernel code too).

Back to the original point, it seems best to write the userspace code
as if we were running against the kernel's exported header file and
the out-of-tree module/interface is just a temporary backport. This is
essentially what is happening today. However, it means that we
shouldn't modify/refactor this file to include OVS-native types,
Windows code, etc. because we wouldn't be able to do that if we were
running against the native kernel.