Re: [PATCH v3 01/16] add util/search-path.{c, h} to test for executables in $PATH

[David Bremner <david@tethera.net>]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> +notmuch_bool_t
> +test_for_executable(const char* exename)
> +{
> +    char *c = NULL, *save = NULL, *tok;
> +    size_t n;
> +    int dfd = -1;

c, dfd, and n could be more meaningful as variable names.

% uncrustify --config devel/uncrustify.cfg --replace util/search-path.c util/search-path.h

yields quite a few whitespace changes (diff attached)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wschanges.diff --]
[-- Type: text/x-diff, Size: 2191 bytes --]

diff --git a/util/search-path.c b/util/search-path.c
index 5eac367..39601e4 100644
--- a/util/search-path.c
+++ b/util/search-path.c
@@ -9,47 +9,47 @@
 
 
 notmuch_bool_t
-test_for_executable(const char* exename)
+test_for_executable (const char *exename)
 {
     char *c = NULL, *save = NULL, *tok;
     size_t n;
     int dfd = -1;
     notmuch_bool_t ret = FALSE;
 
-    if (strchr(exename, '/')) {
-	if (0 == access(exename, X_OK))
+    if (strchr (exename, '/')) {
+	if (0 == access (exename, X_OK))
 	    return TRUE;
 	else
 	    return FALSE;
     }
-    
-    c = getenv("PATH");
+
+    c = getenv ("PATH");
     if (c)
-	c = talloc_strdup(NULL, c);
+	c = talloc_strdup (NULL, c);
     else {
-	n = confstr(_CS_PATH, NULL, 0);
-	c = (char*)talloc_size(NULL, n);
-	if (!c)
+	n = confstr (_CS_PATH, NULL, 0);
+	c = (char *) talloc_size (NULL, n);
+	if (! c)
 	    return FALSE;
-	confstr(_CS_PATH, c, n);
+	confstr (_CS_PATH, c, n);
     }
 
-    tok = strtok_r(c, ":", &save);
+    tok = strtok_r (c, ":", &save);
     while (tok) {
-	dfd = open(tok, O_DIRECTORY | O_RDONLY);
+	dfd = open (tok, O_DIRECTORY | O_RDONLY);
 	if (dfd != -1) {
-	    if (!faccessat(dfd, exename, X_OK, 0)) {
+	    if (! faccessat (dfd, exename, X_OK, 0)) {
 		ret = TRUE;
 		goto done;
 	    }
-	    close(dfd);
+	    close (dfd);
 	}
-	tok = strtok_r(NULL, ":", &save);
+	tok = strtok_r (NULL, ":", &save);
     }
-done:
+  done:
     if (dfd != -1)
-	close(dfd);
+	close (dfd);
     if (c)
-	talloc_free(c);
+	talloc_free (c);
     return ret;
 }
diff --git a/util/search-path.h b/util/search-path.h
index 727d0b3..14c4d14 100644
--- a/util/search-path.h
+++ b/util/search-path.h
@@ -4,13 +4,13 @@
 #include "notmuch.h"
 
 /* can an executable be found with the given name?
- * 
+ *
  * Return TRUE only if we can find something to execute with the
  * associated name.
  *
  * if the name has a '/' in it, we look for it directly with
  * access(exename, X_OK).
- * 
+ *
  * otherwise, we look for it in $PATH (or in confstr(_CS_PATH), if
  * $PATH is unset).
  *
@@ -19,6 +19,6 @@
  */
 
 notmuch_bool_t
-test_for_executable(const char *exename);
+test_for_executable (const char *exename);
 
 #endif

[-- Attachment #3: Type: text/plain, Size: 922 bytes --]


> +    notmuch_bool_t ret = FALSE;
> +
> +    if (strchr(exename, '/')) {
> +	if (0 == access(exename, X_OK))
> +	    return TRUE;
> +	else
> +	    return FALSE;
> +    }
> +    
> +    c = getenv("PATH");
> +    if (c)
> +	c = talloc_strdup(NULL, c);

Is there some advantage to using the talloc_ functions here?

> +    else {

Is n needed outside this block? if not, it could be declared here (and
then I don't care about the single letter name).

> +	n = confstr(_CS_PATH, NULL, 0);

according to a glance at the man page, this might return 0 if there is
no value for _CS_PATH set?


> +
> +    tok = strtok_r(c, ":", &save);
> +    while (tok) {
same comment about block local declaration of dfd

> +	dfd = open(tok, O_DIRECTORY | O_RDONLY);

> +	tok = strtok_r(NULL, ":", &save);

not sure if it helps, but there is also strtok_len in libutil

> +done:

as a real nitpick, we have always (?) used DONE for a label.