GNOME Bugzilla – Bug 740783
[review] move nm_utils_find_helper() to libnm-core
Last modified: 2014-12-05 10:14:39 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.
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>
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 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 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...?
Addressed comments and pushed the patches to: th/file_search_in_path_bgo740783
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.
(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.
Ok, works for me. Looks good.
merged as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=540a30ef96bda06bc99b5479a904434d804e1af1