After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 734131 - [review] dcbw/helper-search: consolidate helper progam searching
[review] dcbw/helper-search: consolidate helper progam searching
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
: 732997 735823 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-08-01 16:29 UTC by Dan Williams
Modified: 2014-09-25 11:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-core-consolidate-helper-progam-searching.patch (29.94 KB, patch)
2014-08-01 16:29 UTC, Dan Williams
none Details | Review
[rebase] core: consolidate helper progam searching (23.75 KB, patch)
2014-08-01 21:13 UTC, Thomas Haller
none Details | Review

Description Dan Williams 2014-08-01 16:29:25 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(-)
Comment 1 Thomas Haller 2014-08-01 21:13:55 UTC
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.
Comment 2 Thomas Haller 2014-08-01 21:51:15 UTC
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
Comment 3 Dan Williams 2014-08-05 17:46:27 UTC
(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?
Comment 4 Dan Williams 2014-08-06 21:09:00 UTC
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
Comment 5 Dan Winship 2014-09-02 12:30:11 UTC
*** Bug 732997 has been marked as a duplicate of this bug. ***
Comment 6 Dan Winship 2014-09-02 12:30:25 UTC
*** Bug 735823 has been marked as a duplicate of this bug. ***
Comment 7 Dan Winship 2014-09-02 12:31:35 UTC
as pointed out in the above two bugs, we're currently hardcoding the path to arping
Comment 8 Dan Williams 2014-09-03 20:48:44 UTC
Rebased and repushed with an additional commit to search for arping.
Comment 9 Dan Winship 2014-09-06 13:19:07 UTC
> 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)
Comment 10 Dan Williams 2014-09-10 14:10:21 UTC
(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.
Comment 11 Dan Winship 2014-09-11 12:27:08 UTC
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
Comment 12 Dan Williams 2014-09-11 17:29:14 UTC
Fixed that up and merged to master.
Comment 13 Pacho Ramos 2014-09-25 11:48:24 UTC
(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