[systemd-devel] [PATCH v2] fsck: Search for fsck.type in PATH

[Zbigniew Jędrzejewski-Szmek]

On Sat, Apr 12, 2014 at 12:07:04PM -0400, Mike Gilbert wrote:
> Modifies find_binary() to accept NULL in the second argument.
> 
> fsck.type lookup logic moved to new fsck_exists() function, with a test.
> ---
>  src/fsck/fsck.c        |  9 ++++-----
>  src/shared/generator.c | 11 ++++-------
>  src/shared/path-util.c | 11 ++++++++---
>  src/shared/util.c      |  8 ++++++++
>  src/shared/util.h      |  2 ++
>  src/test/test-util.c   | 11 +++++++++++
>  6 files changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
> index 18f2aca..520b1a6 100644
> --- a/src/fsck/fsck.c
> +++ b/src/fsck/fsck.c
> @@ -285,14 +285,13 @@ int main(int argc, char *argv[]) {
>  
>          type = udev_device_get_property_value(udev_device, "ID_FS_TYPE");
>          if (type) {
> -                const char *checker = strappenda("/sbin/fsck.", type);
> -                r = access(checker, X_OK);
> +                r = fsck_exists(type);
>                  if (r < 0) {
> -                        if (errno == ENOENT) {
> -                                log_info("%s doesn't exist, not checking file system.", checker);
> +                        if (r == -ENOENT) {
> +                                log_info("fsck.%s doesn't exist, not checking file system.", type);
>                                  return EXIT_SUCCESS;
>                          } else
> -                                log_warning("%s cannot be used: %m", checker);
> +                                log_warning("fsck.%s cannot be used: %s", type, strerror(-r));
>                  }
>          }
>  
> diff --git a/src/shared/generator.c b/src/shared/generator.c
> index 6110303..86d30e7 100644
> --- a/src/shared/generator.c
> +++ b/src/shared/generator.c
> @@ -19,6 +19,7 @@
>    along with systemd; If not, see <http://www.gnu.org/licenses/>.
>  ***/
>  
> +#include <string.h>
>  #include <unistd.h>
>  
>  #include "util.h"
> @@ -45,16 +46,12 @@ int generator_write_fsck_deps(
>          }
>  
>          if (!isempty(fstype) && !streq(fstype, "auto")) {
> -                const char *checker;
>                  int r;
> -
> -                checker = strappenda("/sbin/fsck.", fstype);
> -                r = access(checker, X_OK);
> +                r = fsck_exists(fstype);
>                  if (r < 0) {
> -                        log_warning("Checking was requested for %s, but %s cannot be used: %m", what, checker);
> -
> +                        log_warning("Checking was requested for %s, but fsck.%s cannot be used: %s", what, what, strerror(-r));

The second arg should probably be fstype not what.

>                          /* treat missing check as essentially OK */
> -                        return errno == ENOENT ? 0 : -errno;
> +                        return r == -ENOENT ? 0 : r;
>                  }
>          }
>  
> diff --git a/src/shared/path-util.c b/src/shared/path-util.c
> index bdc54a9..a530dbe 100644
> --- a/src/shared/path-util.c
> +++ b/src/shared/path-util.c
> @@ -425,7 +425,6 @@ int path_is_os_tree(const char *path) {
>  
>  int find_binary(const char *name, char **filename) {
>          assert(name);
> -        assert(filename);
>  
>          if (strchr(name, '/')) {
>                  char *p;
> @@ -437,7 +436,10 @@ int find_binary(const char *name, char **filename) {
>                  if (!p)
>                          return -ENOMEM;
>  
> -                *filename = p;
> +                if (filename)
> +                        *filename = p;
> +                else
> +                        free(p);
Allocating a string just to free it in the next line doesn't make sense.

>                  return 0;
>          } else {
>                  const char *path;
> @@ -464,7 +466,10 @@ int find_binary(const char *name, char **filename) {
>                          }
>  
>                          path_kill_slashes(p);
> -                        *filename = p;
> +                        if (filename)
> +                                *filename = p;
> +                        else
> +                                free(p);
Likewise, no need to mangle the string just to free it.

>  
>                          return 0;
>                  }
> diff --git a/src/shared/util.c b/src/shared/util.c
> index ffe6624..4cdb561 100644
> --- a/src/shared/util.c
> +++ b/src/shared/util.c
> @@ -6391,3 +6391,11 @@ void hexdump(FILE *f, const void *p, size_t s) {
>                  s -= 16;
>          }
>  }
> +
> +int fsck_exists(const char *fstype)
> +{
Brace should go on the end of previous line.

> +        const char *checker;
> +
> +        checker = strappenda("fsck.", fstype);
> +        return find_binary(checker, NULL);
> +}
> diff --git a/src/shared/util.h b/src/shared/util.h
> index 90464c9..45a6f26 100644
> --- a/src/shared/util.h
> +++ b/src/shared/util.h
> @@ -922,3 +922,5 @@ uint64_t physical_memory(void);
>  char* mount_test_option(const char *haystack, const char *needle);
>  
>  void hexdump(FILE *f, const void *p, size_t s);
> +
> +int fsck_exists(const char *fstype);
> diff --git a/src/test/test-util.c b/src/test/test-util.c
> index 93929cd..148e54c 100644
> --- a/src/test/test-util.c
> +++ b/src/test/test-util.c
> @@ -675,6 +675,16 @@ static void test_foreach_string(void) {
>                  assert_se(streq(x, "zzz"));
>  }
>  
> +static void test_fsck_exists(void) {
> +        /* Ensure we use a sane default for PATH. */
> +        unsetenv("PATH");
> +
> +        /* fsck.minix is provided by util-linux and will probably exist. */
> +        assert(fsck_exists("minix") == 0);
> +
> +        assert(fsck_exists("AbCdE") == -ENOENT);
> +}
assert_se().

> +
>  int main(int argc, char *argv[]) {
>          log_parse_environment();
>          log_open();
> @@ -719,6 +729,7 @@ int main(int argc, char *argv[]) {
>          test_hexdump();
>          test_log2i();
>          test_foreach_string();
> +        test_fsck_exists();
>  
>          return 0;
>  }

Zbyszek