[systemd-devel] [PATCH] cryptsetup-generator: auto add deps for device as password

[Zbigniew Jędrzejewski-Szmek]

On Sat, Feb 08, 2014 at 01:12:14PM -0500, Dave Reisner wrote:
> If the password is a device file, we can add Requires/After dependencies
> on the device rather than requiring the user to do so.
> ---
> This is based on a "bug" filed to Arch:
> 
> https://bugs.archlinux.org/task/38842
> 
> Assuming I'm correct about the race condition, this should be an easy way
> of closing it without user involvement.
> 
>  src/cryptsetup/cryptsetup-generator.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cryptsetup/cryptsetup-generator.c b/src/cryptsetup/cryptsetup-generator.c
> index 9c98f0b..4542757 100644
> --- a/src/cryptsetup/cryptsetup-generator.c
> +++ b/src/cryptsetup/cryptsetup-generator.c
> @@ -130,11 +130,26 @@ static int create_disk(
>                      streq(password, "/dev/random") ||
>                      streq(password, "/dev/hw_random"))
>                          fputs("After=systemd-random-seed.service\n", f);
> -                else if (!streq(password, "-") &&
> -                         !streq(password, "none"))
> -                        fprintf(f,
> -                                "RequiresMountsFor=%s\n",
> -                                password);
> +                else {
> +                        _cleanup_free_ char *uu = fstab_node_to_udev_node(password);
> +                        if (uu == NULL)
> +                                return log_oom();
> +
> +                        if (is_device_path(uu)) {
> +                                _cleanup_free_ char *dd = unit_name_from_path(uu, ".device");
> +                                if (dd == NULL)
> +                                        return log_oom();
> +
> +                                fprintf(f,
> +                                                "After=%s\n"
> +                                                "Requires=%s\n",
> +                                                dd, dd);
> +                        } else if (!streq(password, "-") &&
> +                                         !streq(password, "none"))
> +                                        fprintf(f,
> +                                                "RequiresMountsFor=%s\n",
> +                                                password);
> +                }
Patch looks fine, but I don't really understand why you invert the order of conditions.
Something like this feels more natural:

                else if (!streq(password, "-") &&
                         !streq(password, "none")) {

                        _cleanup_free_ char *uu = fstab_node_to_udev_node(password);
                        if (uu == NULL)
                                return log_oom();

                        if (is_device_path(uu)) {
                                _cleanup_free_ char *dd = unit_name_from_path(uu, ".device");
                                if (dd == NULL)
                                        return log_oom();

                                fprintf(f, "After=%1$s\nRequires=%1$s\n", dd);
                        } else
				fprintf(f, "RequiresMountsFor=%s\n", password);
                }

Zbyszek