GNOME Bugzilla – Bug 749510
lr/ancient-ubuntu: Fix build on Ubuntu 12.04
Last modified: 2015-06-02 10:52:55 UTC
It's good reference platform for old libraries. It ships ancient versions we still aim to support, no systemd, etc. If we're able to build there, we can probably build on a potato too. Also, travis-ci uses it -- if we're able to build there we can use it to run a test suite on a more exotic platform than we usually do. The changes needed look reasonably straightforward. 58f6ff7 build: use compat version of g_clear_pointer() 834ebe7 ppp-manager: fix build with Linux 3.2.0 headers 2697aaf systemd-dhcp: fix build with Linux 3.2.0 headers 0fe72f0 libnm-core,libnm-util: avoid calling a constructor 4fbd243 tests: avoid calling GLib.IOChannel.unix_new() 8309229 build: don't abort configure if there's no systemd devel headers d1e0910 build: don't default to -Werror
all patches LGTM
> libnm-core,libnm-util: avoid calling a constructor I get this compile error: nm-setting.c:102:17: error: 'g_type_inited' defined but not used [-Werror=unused-variable] static gboolean g_type_inited = FALSE; ^ Is that variable needed? > systemd-dhcp: fix build with Linux 3.2.0 headers Maybe we should disable the internal DHCP client at build time if we detect that the kernel doesn't have BPF_XOR (which was introduced in 3.7). Or fall back to other clients at runtime? Otherwise I think that someone could select it but it will always fail when trying to load the BPF filter. The rest looks OK to me.
(In reply to Beniamino Galvani from comment #2) > > libnm-core,libnm-util: avoid calling a constructor > > I get this compile error: > > nm-setting.c:102:17: error: 'g_type_inited' defined but not used > [-Werror=unused-variable] > static gboolean g_type_inited = FALSE; > ^ > > Is that variable needed? Yeah, I can't see where it's used? > > systemd-dhcp: fix build with Linux 3.2.0 headers > > Maybe we should disable the internal DHCP client at build time if we detect > that the kernel doesn't have BPF_XOR (which was introduced in 3.7). Or fall > back to other clients at runtime? Otherwise I think that someone could > select it but it will always fail when trying to load the BPF filter. Agreed; if the kernel is too old then internal DHCP will just completely fail. I guess we should just fall back. Maybe there's a way to have the platform do a quick startup check for BPF_XOR somehow?
(In reply to Dan Williams from comment #3) > (In reply to Beniamino Galvani from comment #2) > > > libnm-core,libnm-util: avoid calling a constructor > > > > I get this compile error: > > > > nm-setting.c:102:17: error: 'g_type_inited' defined but not used > > [-Werror=unused-variable] > > static gboolean g_type_inited = FALSE; > > ^ > > > > Is that variable needed? > > Yeah, I can't see where it's used? Fixed > > > systemd-dhcp: fix build with Linux 3.2.0 headers > > > > Maybe we should disable the internal DHCP client at build time if we detect > > that the kernel doesn't have BPF_XOR (which was introduced in 3.7). Or fall > > back to other clients at runtime? Otherwise I think that someone could > > select it but it will always fail when trying to load the BPF filter. > > Agreed; if the kernel is too old then internal DHCP will just completely > fail. I guess we should just fall back. Maybe there's a way to have the > platform do a quick startup check for BPF_XOR somehow? Well, with configure-and-quit we can't really fall back to dhclient. I'd be inclined to say the current error handling is failing is good enough. BPF_XOR is rather recent (kernel 3.7). Systemd bumped their minimum kernel requirement specifically for PBF_XOR. There's a couple more alternatives of what we can do: Systemd's src/shared/missing.h just defines BPF_XOR if it's not defined similar to what we do. We could pull it in and fix the build (which won't fix the runtime). It might still be a good idea to include it to deal with other possible/future dependencies on new headers. Rewrite the filter without BPF_XOR. As simple as s/XOR/SUB/; maybe it's not a good idea to modify the systemd source for this. We could also #define BPF_XOR BPF_SUB, but that's prone to future breakage. (I'd prefer to leave it as it is -- fix the build and not the runtime.)
(In reply to Lubomir Rintel from comment #4) > (In reply to Dan Williams from comment #3) > > (In reply to Beniamino Galvani from comment #2) > > > > systemd-dhcp: fix build with Linux 3.2.0 headers > > > > > > Maybe we should disable the internal DHCP client at build time if we detect > > > that the kernel doesn't have BPF_XOR (which was introduced in 3.7). Or fall > > > back to other clients at runtime? Otherwise I think that someone could > > > select it but it will always fail when trying to load the BPF filter. > > > > Agreed; if the kernel is too old then internal DHCP will just completely > > fail. I guess we should just fall back. Maybe there's a way to have the > > platform do a quick startup check for BPF_XOR somehow? > > Well, with configure-and-quit we can't really fall back to dhclient. I'd be > inclined to say the current error handling is failing is good enough. That only means without internal-plugin "configure-and-quit" must also be disabled. Which would be as simple as: #if !WITH_DHCP_INTERNAL if (nm_config_get_configure_and_quit (nm_config_get ())) g_error ("Not supported"); #endif > BPF_XOR is rather recent (kernel 3.7). Systemd bumped their minimum kernel > requirement specifically for PBF_XOR. > > There's a couple more alternatives of what we can do: > > Systemd's src/shared/missing.h just defines BPF_XOR if it's not defined > similar to what we do. We could pull it in and fix the build (which won't > fix the runtime). It might still be a good idea to include it to deal with > other possible/future dependencies on new headers. > > Rewrite the filter without BPF_XOR. As simple as s/XOR/SUB/; maybe it's not > a good idea to modify the systemd source for this. We could also #define > BPF_XOR BPF_SUB, but that's prone to future breakage. I wouldn't invest time on reimplementing features for old kernels. > (I'd prefer to leave it as it is -- fix the build and not the runtime.) But how about: diff --git i/src/dhcp-manager/nm-dhcp-manager.c w/src/dhcp-manager/nm-dhcp-manager.c index fa562e6..20658f0 100644 --- i/src/dhcp-manager/nm-dhcp-manager.c +++ w/src/dhcp-manager/nm-dhcp-manager.c @@ -33,6 +33,7 @@ #include <unistd.h> #include <fcntl.h> #include <stdio.h> +#include <linux/filter.h> #include "nm-dhcp-manager.h" #include "nm-dhcp-dhclient.h" @@ -400,6 +401,15 @@ nm_dhcp_manager_init (NMDhcpManager *self) } priv->client_type = get_client_type (client, &error); + +#ifndef BPF_XOR + if (priv->client_type == NM_TYPE_DHCP_SYSTEMD) { + /* The systemd interal DHCP library requires recent kernel for + * BPF_XOR. Seems we don't have that which probably will cause a + * runtime failure. */ + nm_log_err (LOGD_DHCP, "DHCP plugin is set to \"internal\" but it seems we have no BPF_XOR"); + } +#endif if (priv->client_type == G_TYPE_INVALID) { nm_log_warn (LOGD_DHCP, "No usable DHCP client found (%s)! DHCP configurations will fail.", error->message);
(In reply to Thomas Haller from comment #5) > I wouldn't invest time on reimplementing features for old kernels. Not that old really. 3.7. > But how about: Well, I don't really like any of these. My biggest problem with that is that you're disabling a runtime feature depending on version of build-time headers. Also, it makes no sense to invent more ways to fail here -- we already fail in a proper manner with EINVAL when the load the bpf program.
(In reply to Lubomir Rintel from comment #6) > (In reply to Thomas Haller from comment #5) > > > I wouldn't invest time on reimplementing features for old kernels. > > Not that old really. 3.7. > > > But how about: > > Well, I don't really like any of these. My biggest problem with that is that > you're disabling a runtime feature depending on version of build-time > headers. > > Also, it makes no sense to invent more ways to fail here -- we already fail > in a proper manner with EINVAL when the load the bpf program. the last suggestions doesn't fail at all. It logs an error, so that the user has a hope to find the issue. Maybe it should be a warning, but it doesn't really hurt.
(In reply to Thomas Haller from comment #7) > (In reply to Lubomir Rintel from comment #6) > > (In reply to Thomas Haller from comment #5) > > > > > I wouldn't invest time on reimplementing features for old kernels. > > > > Not that old really. 3.7. > > > > > But how about: > > > > Well, I don't really like any of these. My biggest problem with that is that > > you're disabling a runtime feature depending on version of build-time > > headers. > > > > Also, it makes no sense to invent more ways to fail here -- we already fail > > in a proper manner with EINVAL when the load the bpf program. > > the last suggestions doesn't fail at all. It logs an error, so that the user > has a hope to find the issue. Maybe it should be a warning, but it doesn't > really hurt. Yes, but it doesn't fix the build and mixes build-time and run-time environments which possesses a risk of being just confusing. If you're convinced this is a good idea, how about logging this when we actually get the EINVAL?
(In reply to Lubomir Rintel from comment #8) > (In reply to Thomas Haller from comment #7) > > (In reply to Lubomir Rintel from comment #6) > > > (In reply to Thomas Haller from comment #5) > > > > > > > I wouldn't invest time on reimplementing features for old kernels. > > > > > > Not that old really. 3.7. > > > > > > > But how about: > > > > > > Well, I don't really like any of these. My biggest problem with that is that > > > you're disabling a runtime feature depending on version of build-time > > > headers. > > > > > > Also, it makes no sense to invent more ways to fail here -- we already fail > > > in a proper manner with EINVAL when the load the bpf program. > > > > the last suggestions doesn't fail at all. It logs an error, so that the user > > has a hope to find the issue. Maybe it should be a warning, but it doesn't > > really hurt. > > Yes, but it doesn't fix the build Of course logging a warning in addition to fix the build :) The build should be fixed in any case. > and mixes build-time and run-time > environments which possesses a risk of being just confusing. If you're > convinced this is a good idea, how about logging this when we actually get > the EINVAL? If you can identify the exact location that is of course even better. But it seems hard to know that a failure was caused by EINVAL due to ~that~ particular issue.
I agree that we should at least try to print a meaningful error that describes the issue with BPF_XOR when the loading of the filter fails. So for example we could: - implement get_path_func() of internal client's ClientDesc, which is used to check if the client is available; we could try to load a simple filter with BPF_XOR and return error when it fails - or simply modify systemd's dhcp-network.c to log a detailed error when the loading of the filter fails
(In reply to Thomas Haller from comment #9) > (In reply to Lubomir Rintel from comment #8) > > (In reply to Thomas Haller from comment #7) > > > (In reply to Lubomir Rintel from comment #6) > > > > (In reply to Thomas Haller from comment #5) > > > > > > > > > I wouldn't invest time on reimplementing features for old kernels. > > > > > > > > Not that old really. 3.7. > > > > > > > > > But how about: > > > > > > > > Well, I don't really like any of these. My biggest problem with that is that > > > > you're disabling a runtime feature depending on version of build-time > > > > headers. > > > > > > > > Also, it makes no sense to invent more ways to fail here -- we already fail > > > > in a proper manner with EINVAL when the load the bpf program. > > > > > > the last suggestions doesn't fail at all. It logs an error, so that the user > > > has a hope to find the issue. Maybe it should be a warning, but it doesn't > > > really hurt. > > > > Yes, but it doesn't fix the build > > Of course logging a warning in addition to fix the build :) > The build should be fixed in any case. Well, that's what the original patch was supposed to do. Is there a better way to do that than what the original patch did? > > and mixes build-time and run-time > > environments which possesses a risk of being just confusing. If you're > > convinced this is a good idea, how about logging this when we actually get > > the EINVAL? > > If you can identify the exact location that is of course even better. But it > seems hard to know that a failure was caused by EINVAL due to ~that~ > particular issue. Exactly this. We can not do much better than we already do. (In reply to Beniamino Galvani from comment #10) > I agree that we should at least try to print a meaningful error that > describes the issue with BPF_XOR when the loading of the filter fails. So > for example we could: > > - implement get_path_func() of internal client's ClientDesc, which is used > to check if the client is available; we could try to load a simple filter > with BPF_XOR and return error when it fails I believe this is a not a very good idea. There's a lot more than BPF_XOR that can go wrong with use of kernel interfaces all around network manager and heuristically determining what features are available sounds horribly overengineered. If only decide to do the runtime check for BPF_XOR then we'd probably need a reason why do we nitpick on that particular one. > - or simply modify systemd's dhcp-network.c to log a detailed error when the > loading of the filter fails The error is already as detailed as possible. We have no other details on why precisely did the filter fail to load other than the EINVAL.
(In reply to Lubomir Rintel from comment #11) > (In reply to Thomas Haller from comment #9) > > (In reply to Lubomir Rintel from comment #8) > > > (In reply to Thomas Haller from comment #7) > > > > (In reply to Lubomir Rintel from comment #6) > > > > > (In reply to Thomas Haller from comment #5) > > > > > > > > > > > I wouldn't invest time on reimplementing features for old kernels. > > > > > > > > > > Not that old really. 3.7. > > > > > > > > > > > But how about: > > > > > > > > > > Well, I don't really like any of these. My biggest problem with that is that > > > > > you're disabling a runtime feature depending on version of build-time > > > > > headers. > > > > > > > > > > Also, it makes no sense to invent more ways to fail here -- we already fail > > > > > in a proper manner with EINVAL when the load the bpf program. > > > > > > > > the last suggestions doesn't fail at all. It logs an error, so that the user > > > > has a hope to find the issue. Maybe it should be a warning, but it doesn't > > > > really hurt. > > > > > > Yes, but it doesn't fix the build > > > > Of course logging a warning in addition to fix the build :) > > The build should be fixed in any case. > > Well, that's what the original patch was supposed to do. Is there a better > way to do that than what the original patch did? the original patch works around the build failure by defining BPF_XOR. I merely suggest in comment 5 to additionally log a warning when: - somebody is trying to use internal dhcp client - and, we suspect later failures due to missing BPF_XOR. > > > > and mixes build-time and run-time > > > environments which possesses a risk of being just confusing. If you're > > > convinced this is a good idea, how about logging this when we actually get > > > the EINVAL? > > > > If you can identify the exact location that is of course even better. But it > > seems hard to know that a failure was caused by EINVAL due to ~that~ > > particular issue. > > Exactly this. We can not do much better than we already do. Well, currently we don't really log a helpful message. dhcp failed due to: invalid argument is not very helpful for the user to understand. Anyway, regardless of whether you do comment 5, the branch LGTM.
(In reply to Thomas Haller from comment #12) > (In reply to Lubomir Rintel from comment #11) > > (In reply to Thomas Haller from comment #9) > > > (In reply to Lubomir Rintel from comment #8) > > > > (In reply to Thomas Haller from comment #7) > > > > > (In reply to Lubomir Rintel from comment #6) > > > > > > (In reply to Thomas Haller from comment #5) > > > > > > > > > > > > > I wouldn't invest time on reimplementing features for old kernels. > > > > > > > > > > > > Not that old really. 3.7. > > > > > > > > > > > > > But how about: > > > > > > > > > > > > Well, I don't really like any of these. My biggest problem with that is that > > > > > > you're disabling a runtime feature depending on version of build-time > > > > > > headers. > > > > > > > > > > > > Also, it makes no sense to invent more ways to fail here -- we already fail > > > > > > in a proper manner with EINVAL when the load the bpf program. > > > > > > > > > > the last suggestions doesn't fail at all. It logs an error, so that the user > > > > > has a hope to find the issue. Maybe it should be a warning, but it doesn't > > > > > really hurt. > > > > > > > > Yes, but it doesn't fix the build > > > > > > Of course logging a warning in addition to fix the build :) > > > The build should be fixed in any case. > > > > Well, that's what the original patch was supposed to do. Is there a better > > way to do that than what the original patch did? > > the original patch works around the build failure by defining BPF_XOR. > I merely suggest in comment 5 to additionally log a warning when: > - somebody is trying to use internal dhcp client > - and, we suspect later failures due to missing BPF_XOR. But do we suspect that? I don't. Could we add a build-time #warning instead? > > > > and mixes build-time and run-time > > > > environments which possesses a risk of being just confusing. If you're > > > > convinced this is a good idea, how about logging this when we actually get > > > > the EINVAL? > > > > > > If you can identify the exact location that is of course even better. But it > > > seems hard to know that a failure was caused by EINVAL due to ~that~ > > > particular issue. > > > > Exactly this. We can not do much better than we already do. > > Well, currently we don't really log a helpful message. > dhcp failed due to: invalid argument > is not very helpful for the user to understand. Well, that's as helpful as the diagnostics from kernel is. Would something like "failed to load the BPF filter" or "Your kernel version is not able to run filter the internal DHCP client uses" be more useful? If we're modifying the internal dhcp client for this, wouldn't it just make sense to fix the filter so that it works on older kernels instead? It's even more simpler to do. > Anyway, regardless of whether you do comment 5, the branch LGTM. 83a82b4 merge: fix build & tests with older tooling 8402145 build: don't default to -Werror e486a38 build: don't abort configure if there's no systemd devel headers b9b7bb1 tests: avoid calling GLib.IOChannel.unix_new() ccb0ca4 libnm-core,libnm-util: avoid calling a constructor 3811a68 systemd-dhcp: fix build with Linux 3.2.0 headers 22b99e3 ppp-manager: fix build with Linux 3.2.0 headers I've pushed it as it is; please feel free to add a more clever XOR_BPF warning. I can't really figure out something that would not mix up build-time and run-time checks with the risk of bogus warning.