netlink: include opt_netlink.h

[kp]

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

[markj]

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.

[markj]

In D42170#962264, @markj wrote:

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.

[kp]

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.

[melifaro]

In D42170#962265, @markj wrote:

In D42170#962264, @markj wrote:

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]

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.

[melifaro]

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)

[imp]

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

[melifaro]

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..

[imp]

In D42170#962335, @melifaro wrote:

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...

[imp]

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?

[kp]

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.

[imp]

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).

[kp]

D42179 was committed instead.