From: David Bremner Date: Tue, 9 Feb 2016 12:57:15 +0000 (+2000) Subject: Re: [PATCH v3 01/16] add util/search-path.{c, h} to test for executables in $PATH X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=6456e8384a4ea7c92aaf0407243b612990d7856e;p=notmuch-archives.git Re: [PATCH v3 01/16] add util/search-path.{c, h} to test for executables in $PATH --- diff --git a/2f/2daed5a44a94352f348afe291cc8b501874a9d b/2f/2daed5a44a94352f348afe291cc8b501874a9d new file mode 100644 index 000000000..fe32f7afe --- /dev/null +++ b/2f/2daed5a44a94352f348afe291cc8b501874a9d @@ -0,0 +1,220 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by arlo.cworth.org (Postfix) with ESMTP id 6EED66DE0AC2 + for ; Tue, 9 Feb 2016 04:57:24 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at cworth.org +X-Spam-Flag: NO +X-Spam-Score: -0.308 +X-Spam-Level: +X-Spam-Status: No, score=-0.308 tagged_above=-999 required=5 tests=[AWL=0.243, + RP_MATCHES_RCVD=-0.55, SPF_PASS=-0.001] autolearn=disabled +Received: from arlo.cworth.org ([127.0.0.1]) + by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id NuYj2t_V0f9o for ; + Tue, 9 Feb 2016 04:57:22 -0800 (PST) +Received: from fethera.tethera.net (fethera.tethera.net [198.245.60.197]) + by arlo.cworth.org (Postfix) with ESMTPS id 3E0046DE02CE + for ; Tue, 9 Feb 2016 04:57:21 -0800 (PST) +Received: from remotemail by fethera.tethera.net with local (Exim 4.84) + (envelope-from ) + id 1aT7qP-0000H3-80; Tue, 09 Feb 2016 07:56:37 -0500 +Received: (nullmailer pid 10963 invoked by uid 1000); + Tue, 09 Feb 2016 12:57:15 -0000 +From: David Bremner +To: Daniel Kahn Gillmor , + Notmuch Mail +Subject: Re: [PATCH v3 01/16] add util/search-path.{c, + h} to test for executables in $PATH +In-Reply-To: <1454272801-23623-2-git-send-email-dkg@fifthhorseman.net> +References: <1454272801-23623-1-git-send-email-dkg@fifthhorseman.net> + <1454272801-23623-2-git-send-email-dkg@fifthhorseman.net> +User-Agent: Notmuch/0.21+5~gca076ce (http://notmuchmail.org) Emacs/24.5.1 + (x86_64-pc-linux-gnu) +Date: Tue, 09 Feb 2016 08:57:15 -0400 +Message-ID: <87oabqvy6s.fsf@maritornes.cs.unb.ca> +MIME-Version: 1.0 +Content-Type: multipart/mixed; boundary="=-=-=" +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.20 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Tue, 09 Feb 2016 12:57:24 -0000 + +--=-=-= +Content-Type: text/plain + +Daniel Kahn Gillmor 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) + + +--=-=-= +Content-Type: text/x-diff +Content-Disposition: inline; filename=wschanges.diff + +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 + +--=-=-= +Content-Type: text/plain + + +> + 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. + +--=-=-=--