Page MenuHomeFreeBSD

netlink: include opt_netlink.h
AbandonedPublic

Authored by kp on Oct 12 2023, 5:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 6:55 AM
Unknown Object (File)
Jan 21 2024, 8:06 AM
Unknown Object (File)
Jan 5 2024, 10:32 PM
Unknown Object (File)
Dec 25 2023, 5:35 PM
Unknown Object (File)
Dec 25 2023, 5:21 PM
Unknown Object (File)
Dec 23 2023, 12:15 AM
Unknown Object (File)
Nov 30 2023, 4:25 PM
Unknown Object (File)
Oct 17 2023, 1:17 PM

Details

Reviewers
markj
melifaro
bapt
imp
Group Reviewers
network
Summary

Include the opt_netlink.h file directly from netlink.h. This
ensures that the NETLINK define is correctly set. If not we may
end up with unloadable modules, due to missing symbols (such as
nlmsg_get_group_writer).

Including the opt_netlink.h file directly in netlink.h ensures that
netlink users do not all need to remember to do this include.

PR: 274306
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 53942
Build 50832: arc lint + arc unit

Event Timeline

kp requested review of this revision.Oct 12 2023, 5:29 PM
This revision is now accepted and ready to land.Oct 12 2023, 5:36 PM

One caveat of this approach is that netlink/netlink.h has to be included before all headers which might expect NETLINK to be defined. For instance, it'll be a bug for .c files to include netlink.h after netlink_ctl.h. Maybe the solution there is to make netlink_ctl.h include opt_netlink.h as well, I'm not sure.

One caveat of this approach is that netlink/netlink.h has to be included before all headers which might expect NETLINK to be defined. For instance, it'll be a bug for .c files to include netlink.h after netlink_ctl.h. Maybe the solution there is to make netlink_ctl.h include opt_netlink.h as well, I'm not sure.

Another solution is to just define NETLINK in opt_global.h. I've never found having separate option headers to be useful.

Should we do the include in the headers that directly need it instead? That'd be at least netlink_ctl.h, netlink_message_writer.h and route/route_var.h.

One caveat of this approach is that netlink/netlink.h has to be included before all headers which might expect NETLINK to be defined. For instance, it'll be a bug for .c files to include netlink.h after netlink_ctl.h. Maybe the solution there is to make netlink_ctl.h include opt_netlink.h as well, I'm not sure.

Another solution is to just define NETLINK in opt_global.h. I've never found having separate option headers to be useful.

I was thinking in a similar way, having opt_proto.h which contains defines for INET, INET6 and NETLINK, so a whole variety of errors would be eliminated..

imp requested changes to this revision.EditedOct 12 2023, 7:12 PM

I think this is a terrible idea. .h files should almost never include opt files. It makes them harder to use to build a module against.

This revision now requires changes to proceed.Oct 12 2023, 7:12 PM
In D42170#962271, @kp wrote:

Should we do the include in the headers that directly need it instead? That'd be at least netlink_ctl.h, netlink_message_writer.h and route/route_var.h.

This seem a bit contrary to the approach used in the other parts of the kernel. If really want to switch the pattern, I’d ether vote for opt_proto ( or opt_global.h)

opt_global.h is ok. Opt_proto just moves th problem.

In D42170#962312, @imp wrote:

opt_global.h is ok. Opt_proto just moves th problem.

But we actually have it for inet/inet6 as well. I ran into the similar issues many times..

In D42170#962312, @imp wrote:

opt_global.h is ok. Opt_proto just moves th problem.

But we actually have it for inet/inet6 as well. I ran into the similar issues many times..

That is also broken. At least with that the modules that need it include opt_inet.h for ages.... it's one og the warts in the build system I've been trying to get rid of.

For netlink, if I understand correctly, the issue is a lot of things need to know that aren't very connected to netlink or they get undefined symbols because the inlines weren't expanded and/or they don't have the right module dependencies... hence the desire to "hide" this issue in netlink.h...

But I'll ask the radical question: we had netlink as an option for transitioning to it since it was so large and we were late in the release cycle. None of that is really true anymore. Can we just unifdef -DNETLINK and kill the option?

In D42170#962312, @imp wrote:

opt_global.h is ok. Opt_proto just moves th problem.

See https://reviews.freebsd.org/D42179

I'm not opposed to removing the switch altogether, especially because pf relies on netlink too now (and I hope to move all pf control code to netlink at some point), but I'm not sure right before 14.0 is the time to change that. This could potentially go in still, but I expect resistance to removing the switch entirely.

I like the plan "move it to opt_global.h now, MFC that and work on a longer term solution of removing the need for knowing if NETLINK is defined or not in the headers (lots of ways to do this).

D42179 was committed instead.