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 740783 - [review] move nm_utils_find_helper() to libnm-core
[review] move nm_utils_find_helper() to libnm-core
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-11-26 20:56 UTC by Thomas Haller
Modified: 2014-12-05 10:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
libnm: add function nm_utils_file_search_in_paths() (5.60 KB, patch)
2014-11-26 20:57 UTC, Thomas Haller
reviewed Details | Review
core: implement nm_utils_find_helper() based on nm_utils_file_search_in_paths() (3.31 KB, patch)
2014-11-26 20:57 UTC, Thomas Haller
reviewed Details | Review

Description Thomas Haller 2014-11-26 20:56:21 UTC
I think this function is useful also for other clients. For example VPN plugins frequently call out to other binaries.

In order to be generally useful, there are now more options.
Comment 1 Thomas Haller 2014-11-26 20:57:10 UTC
Created attachment 291592 [details] [review]
libnm: add function nm_utils_file_search_in_paths()

We now also use a similar function in VPN plugins. It makes
sense to provide a generic implementation in libnm.

Signed-off-by: Thomas Haller <thaller@redhat.com>
Comment 2 Thomas Haller 2014-11-26 20:57:16 UTC
Created attachment 291593 [details] [review]
core: implement nm_utils_find_helper() based on nm_utils_file_search_in_paths()

This also changes behavior in that we now only find files that
are executable.
Comment 3 Dan Winship 2014-12-01 11:30:51 UTC
Comment on attachment 291592 [details] [review]
libnm: add function nm_utils_file_search_in_paths()

> We now also use a similar function in VPN plugins.

Note that the VPN plugins aren't going to be ported to libnm right away, so maybe we want to add this to libnm-util too?

>+ * @try_first: (allow-none): a custom path to try first before searching.
>+ *   It is silently ignored it it is empty or not an absolute path.

"IF it is"

I assume that the reason for silently ignoring it is that it just makes it simpler to plug in the results of the configure checks that way?

>+ * @file_test_flags: the flags passed to g_file_test() when searching
>+ *   for @progname. Set it to 0 to skip the g_file_test().

why would this ever be anything besides G_FILE_TEST_IS_EXECUTABLE?

>+ * Searches for the @progname in common system paths.

Not true, it searches for @progname in @paths (which need not be common)

>+	if (   try_first
>+	    && try_first[0] == '/'
>+	    && (file_test_flags == 0 && g_file_test (try_first, file_test_flags))
>+	    && (!predicate || predicate (try_first, user_data)))
>+		return g_intern_string (try_first);

Should be "file_test_flags != 0", not "== 0". (Or else "||" instead of "&&", as below.)

>+	/* For now, just intern the returned string. The API documentation leaves it
>+	 * open to cache the value in a static (per-thread?) variable to avoid
>+	 * memory-leaks when calling excessively often with different file names. */

I think it's fine to just say that we intern the string. You are not going to need to locate an excessively large number of helper programs.

>+	g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND, "Could not find %s binary", progname);

Maybe localize, and put quotes around the %s.
Comment 4 Dan Winship 2014-12-01 11:32:24 UTC
Comment on attachment 291593 [details] [review]
core: implement nm_utils_find_helper() based on nm_utils_file_search_in_paths()

>+extern const char *const NM_PATHS_DEFAULT[];

That doesn't actually seem to be needed...?
Comment 5 Thomas Haller 2014-12-01 12:25:07 UTC
Addressed comments and pushed the patches to:
  th/file_search_in_path_bgo740783
Comment 6 Dan Williams 2014-12-03 21:52:11 UTC
I'm on the fence about it still, but I don't especially like exposing this to *all* clients of libnm since it's rather generic stuff and not related to NetworkManager at all.  But we combined libnm-vpn into libnm, so now we have no great way to share this with only the VPN plugins.

I'd settle for marking the function as "PRIVATE API DON'T USE THIS IT WILL CHANGE" in the documentation and headers though...  at least then we have an excuse later.
Comment 7 Thomas Haller 2014-12-04 12:40:50 UTC
(In reply to comment #6)
> I'm on the fence about it still, but I don't especially like exposing this to
> *all* clients of libnm since it's rather generic stuff and not related to
> NetworkManager at all.  But we combined libnm-vpn into libnm, so now we have no
> great way to share this with only the VPN plugins.
> 
> I'd settle for marking the function as "PRIVATE API DON'T USE THIS IT WILL
> CHANGE" in the documentation and headers though...  at least then we have an
> excuse later.

Hm. I think it is a small enough function with a general purpose. Effectively we cannot remove it later, so the PRIVATE API comment does not help there.

How could this function turn out not be useful in the future? But even then, in the worst case we have 1k unused code.
Comment 8 Dan Williams 2014-12-05 01:10:14 UTC
Ok, works for me.  Looks good.