GNOME Bugzilla – Bug 734131
[review] dcbw/helper-search: consolidate helper progam searching
Last modified: 2014-09-25 11:48:24 UTC
Created attachment 282286 [details] [review] 0001-core-consolidate-helper-progam-searching.patch Instead of having basically the same code in a bunch of different place to find helper programs, just have one place do it. Yes, this does mean that the same sequence of paths is searched for all helpers (so for example, dnsmasq will no longer be found first in /usr/local) but I think consistency is the better option here. --- src/NetworkManagerUtils.c | 61 ++++++++++++++++++++++++++++++++ src/NetworkManagerUtils.h | 6 ++++ src/devices/nm-device.c | 42 +++++++++------------- src/devices/team/nm-device-team.c | 31 ++++++---------- src/dhcp-manager/nm-dhcp-dhclient.c | 40 ++++++++------------- src/dhcp-manager/nm-dhcp-dhclient.h | 2 +- src/dhcp-manager/nm-dhcp-dhcpcd.c | 42 ++++++++-------------- src/dhcp-manager/nm-dhcp-dhcpcd.h | 2 +- src/dhcp-manager/nm-dhcp-manager.c | 15 +++----- src/dns-manager/nm-dns-dnsmasq.c | 32 ++++++----------- src/dnsmasq-manager/nm-dnsmasq-manager.c | 36 +++++-------------- src/nm-dcb.c | 33 ++++------------- src/ppp-manager/nm-ppp-manager.c | 50 +++++--------------------- 13 files changed, 166 insertions(+), 226 deletions(-)
Created attachment 282292 [details] [review] [rebase] core: consolidate helper progam searching The patch from attachment 282286 [details] [review] no longer applies (it applied on commit fa809c2636c7d061c598fe84bbc2aa6fcb0e32b1). I rebased it and resolved the conflicts. Not it applies on commit 833ff17cd76c40bd1b227b27b2736745d147e733.
You say: Yes, this does mean that the same sequence of paths is searched for all helpers (so for example, dnsmasq will no longer be found first in /usr/local) but I think consistency is the better option here. how about passing the search path (char**) as argument, and (when passing NULL) fallback to the one starting with /sbin/? you could define two extern variables: extern const char *const*NM_UTILS_FIND_PATH_SBIN; extern const char *const*NM_UTILS_FIND_PATH_BIN; ? Also, + static const Path paths[] = { + /* NOTE: update 'buf' size if adding longer paths */ + MAKE_PATH("/sbin/"), + MAKE_PATH("/usr/sbin/"), I wouldn't bother storing the length too. Just make it char**. Also the construction for: + g_assert (strlen (progname) < 30); + for (i = 0; i < G_N_ELEMENTS (paths); i++) { + static char buf[50]; + const guint plen = paths[i].len; + const guint left = sizeof (buf) - plen; seems a bit micro optimized. I would reuse a GString to construct the paths. in aipd_start (): + static const char *aipd_binary = NULL; I would not cache the path. I means that once NM found the path, it would not search again until restart. Same for other places (teamd_start). + /* If dhclient was disabled at build time, DHCLIENT_PATH will be empty */ + if (!DHCLIENT_PATH[0]) + return NULL; this means, you cannot build NM to just use the search paths. You always *have* to configure a try_first value. Rest looks good
(In reply to comment #2) > You say: > > Yes, this > does mean that the same sequence of paths is searched for all helpers > (so for example, dnsmasq will no longer be found first in /usr/local) > but I think consistency is the better option here. > > how about passing the search path (char**) as argument, and (when passing NULL) > fallback to the one starting with /sbin/? I thought about passing const char ** as an argument, but I don't really think it's a problem to use the same search list for everything actually. I think we should make the change and see if anyone cares that now /usr/sbin/foo gets used instead of /usr/local/sbin/foo. > Also, + static const Path paths[] = { > + /* NOTE: update 'buf' size if adding longer paths */ > + MAKE_PATH("/sbin/"), > + MAKE_PATH("/usr/sbin/"), > > I wouldn't bother storing the length too. Just make it char**. > Also the construction for: > > + g_assert (strlen (progname) < 30); > + for (i = 0; i < G_N_ELEMENTS (paths); i++) { > + static char buf[50]; > + const guint plen = paths[i].len; > + const guint left = sizeof (buf) - plen; > > seems a bit micro optimized. I would reuse a GString to construct the paths. Yeah, you could be right. I just did that to avoid a strlen() and memory allocation every time we do a search path. WRT allocations though, see below: > in aipd_start (): > + static const char *aipd_binary = NULL; > > I would not cache the path. I means that once NM found the path, it would not > search again until restart. Same for other places (teamd_start). Correct. I'm not sure this is a problem, at least for package-based distributions, where dependencies should ensure that the binary exists at the path. However, if we do intend NetworkManager to run for a long time, maybe it should resilient against other helpers being added/removed. I only chose this approach to minimze allocation/deallocation, since the previous code for all this used static strings, while the new code uses allocated ones. I could make them all alloc/free (well, using gs_free :) which I guess would be fine with me. > + /* If dhclient was disabled at build time, DHCLIENT_PATH will be empty */ > + if (!DHCLIENT_PATH[0]) > + return NULL; > > this means, you cannot build NM to just use the search paths. You always *have* > to configure a try_first value. This comes from configure and preserves previous behavior. I believe the intention there was that if you didn't want to ever run dhcpcd with NM even if it was installed, you could disable that, and ensure that NM never fell back to dhcpcd. This was used in conjunction with leaving "dhcp=" blank in NM.conf to get the default DHCP client enabled at configure time. If we did not do this (!*_PATH[0]) return NULL; behavior, then if the user had disabled dhclient at build time, NM would still search dhclient first and find the default locations (from the search paths) instead of ignoring dhclient and using dhcpcd instead. I suppose we could change configure to indicate explicitly that a client was disabled, and then we could get rid of the path search stuff in configure.ac and if the user didn't specify a path but didn't disable the client, NM would just search for it?
I reworked the patch, and pushed it to the dcbw/helper-search branch. Changes: 1) reworked configure-time DHCP client path handling to explicitly disable DHCP clients if desired 2) changed nm_utils_find_helper() to use a GString instead of a static buffer and remove some micro-optimizations 3) stopped using 'static const char *' for storing found helpers, instead check for the helper each time it is used
*** Bug 732997 has been marked as a duplicate of this bug. ***
*** Bug 735823 has been marked as a duplicate of this bug. ***
as pointed out in the above two bugs, we're currently hardcoding the path to arping
Rebased and repushed with an additional commit to search for arping.
> core: consolidate helper progam searching (bgo #734131) >+ * @error_domain: domain for a "not found" error >+ * @error_code: code for a "not found" error >+ * @error: on failure, a "not found" error using @error_domain and @error_code These are used in fewer than half of the calls... are the added junk "NULL, 0, NULL"s worth the removed g_set_error()s? >+ static const char *paths[] = { >+ "/sbin/", >+ "/usr/sbin/", >+ "/usr/local/sbin/", >+ "/usr/bin/", >+ "/usr/local/bin/", >+ "/usr/pkg/sbin/", >+ }; /usr/pkg isn't used on any Linux system is it? (And why /usr/pkg/sbin but not /usr/pkg/bin?) It seems like we should either include $PREFIX/sbin and $PREFIX/bin, or else have a configure --with-helper-prefix option Even if you're not going to cache the paths, you could still intern them, so that the return value is const, which would simplify things for the callers. (In normal cases, this would just be one interned string for each helper we use.) >- _("'dhclient' could be found.")); >+ _("'dhclient' could be found or disabled.")); "'dhclient' could not be found, or was disabled" (likewise dhcpcd)
(In reply to comment #9) > > core: consolidate helper progam searching (bgo #734131) > > >+ * @error_domain: domain for a "not found" error > >+ * @error_code: code for a "not found" error > >+ * @error: on failure, a "not found" error using @error_domain and @error_code > > These are used in fewer than half of the calls... are the added junk "NULL, 0, > NULL"s worth the removed g_set_error()s? And the errors aren't pushed over DBus, but are only printed and freed, so I just changed it to G_IO_ERROR_NOT_FOUND and removed the domain/code parameters. > >+ static const char *paths[] = { > >+ "/sbin/", > >+ "/usr/sbin/", > >+ "/usr/local/sbin/", > >+ "/usr/bin/", > >+ "/usr/local/bin/", > >+ "/usr/pkg/sbin/", > >+ }; > > /usr/pkg isn't used on any Linux system is it? (And why /usr/pkg/sbin but not > /usr/pkg/bin?) I think it was for pkgsrc I guess, but the commit logs from it don't show why it got added. Since nobody actually requested it, I'll just remove it and wait for somebody to yell. > It seems like we should either include $PREFIX/sbin and $PREFIX/bin, or else > have a configure --with-helper-prefix option Done. > Even if you're not going to cache the paths, you could still intern them, so > that the return value is const, which would simplify things for the callers. > (In normal cases, this would just be one interned string for each helper we > use.) Done. > >- _("'dhclient' could be found.")); > >+ _("'dhclient' could be found or disabled.")); > > "'dhclient' could not be found, or was disabled" (likewise dhcpcd) Done. Re-pushed.
create_dm_cmd_line() in nm-dnsmasq-manager.c now leaks cmd in the "Failed to find DHCP address ranges" case. everything else looks good
Fixed that up and merged to master.
(In reply to comment #12) > Fixed that up and merged to master. Could this be backported to nm-0-9-10 branch to get it fixed in next 0.9.10.x release? Thanks