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

Mike Gilbert floppym at gentoo.org
Sat Apr 12 12:10:54 PDT 2014


On Sat, Apr 12, 2014 at 1:40 PM, Zbigniew Jędrzejewski-Szmek
<zbyszek at in.waw.pl> wrote:
> 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.
>

Right, I'll fix that.

>>                          /* 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.
>

Yeah, this one could probably be adjusted not to do that. I was trying
to change as little as possible.

>>                  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.
>

Ah, yeah. I guess this would work:

if (filename)
        path_kill_slashes(p);
        *filename = p;
else
        free(p);

>>
>>                          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().
>

We are comparing integers, not strings. This matches the other integer
comparisons in this source file.

>> +
>>  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


More information about the systemd-devel mailing list