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

[Lennart Poettering]

On Wed, 09.04.14 10:07, Mike Gilbert (floppym at gentoo.org) wrote:

> Matches default behavior in recent util-linux.

Quite frankly, this is really really broken in Gentoo. Randomly moving
packages from /sbin to /usr/sbin that are required during early boot is
just wrong. Either you keep the distinction between the two dirs, and
then fsck clearly belongs in /sbin, and not /usr/sbin, or you remove the
distinction, and then /sbin should be a symlink to /usr/sbin so that the
distinction doesn't matter.

But this scheme that Gentoo is following there, is just a recipe for
breaking almost every possible package. 

The /usr merge is about increasing compatibility by providing everything
in both dirs. But Gentoo is really decreasing compatibility with itself
here by randomly moving things around and not providing things under the
old location. Really, you should rethink this, it's bogus! Entirely and
completely bogus!

Anyway, even though I strongl disagree with how Gentoo is handling the
transition here, the patch looks OK and should probably go in.

> ---
>  src/fsck/fsck.c        | 6 ++++--
>  src/shared/generator.c | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
> index 18f2aca..24c8890 100644
> --- a/src/fsck/fsck.c
> +++ b/src/fsck/fsck.c
> @@ -36,6 +36,7 @@
>  #include "bus-error.h"
>  #include "bus-errors.h"
>  #include "fileio.h"
> +#include "path-util.h"
>  #include "udev-util.h"
>  
>  static bool arg_skip = false;
> @@ -285,8 +286,9 @@ 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);
> +                const char *checker = strappenda("fsck.", type);


We need to be a lot more careful with alloca() here. It's not OK to use
alloca() to initialize newly declared variables, and it's not OK to use
alloca() as parameter to other function calls. Since alloca() plays
games with the stack pointer, and variable decalration as well as
funciton invocation also do that one should really not mix them.

Or in other words:

Please use:

       const char *checker;
       checker = strappenda("fsck.", type);

Rather than:

       const char *checker = strappenda("fsck.", type);

I understand that the code is already broken here, and that this is not
a bug in your patch, but I would appreciate if you patch would fix this
as side effect.

(Also, completely orthogonal to the alloca() discussion, I think we
should avoid mixing function invocations and variable declarations in
the same line. Strictly speaking alloca() isn't a function, but it
feelds like one. Initializing a newly delcared variable to some constat
value is totally fine and a good thing, but invoking functions there is
something we should avoid.

> +                _cleanup_free_ char *command = NULL;
> +                r = find_binary(checker, &command);


Given that we are not intrested in the actual path for the command, I'd
prefer if find_binary() could be fixed to handle a NULL command
argument, in which case it should just not return the path but still
return useful error codes.

>                  if (r < 0) {
>                          if (errno == ENOENT) {
>                                  log_info("%s doesn't exist, not checking file system.", checker);
> diff --git a/src/shared/generator.c b/src/shared/generator.c
> index 6110303..6f4eaae 100644
> --- a/src/shared/generator.c
> +++ b/src/shared/generator.c
> @@ -24,6 +24,7 @@
>  #include "util.h"
>  #include "special.h"
>  #include "mkdir.h"
> +#include "path-util.h"
>  #include "unit-name.h"
>  #include "generator.h"
>  
> @@ -46,10 +47,11 @@ int generator_write_fsck_deps(
>  
>          if (!isempty(fstype) && !streq(fstype, "auto")) {
>                  const char *checker;
> +                _cleanup_free_ char *command = NULL;
>                  int r;
>  
> -                checker = strappenda("/sbin/fsck.", fstype);
> -                r = access(checker, X_OK);
> +                checker = strappenda("fsck.", fstype);
> +                r = find_binary(checker, &command);
>                  if (r < 0) {
>                          log_warning("Checking was requested for %s,
> -                but %s cannot be used: %m", what, checker);

Given that the logic for looking for an fsck is used multiple times and
is not entirely trivial now I'd prefer if we could move this to util.c
and provide a simple call

bool fsck_exists(const char *fstype);

Or so, that does the right thing internally and hides it to its users.

Lennart

-- 
Lennart Poettering, Red Hat