[systemd-devel] [PATCH] cryptsetup: craft a unique ID with the source device

[Lennart Poettering]

On Fri, 29.05.15 12:17, harald at redhat.com (harald at redhat.com) wrote:

>  
> +static char* disk_maj_min(const char *path) {

I'd really not abbreivate things here... call it "major" and
"minor". there's no point in saving two "or"s...

> +        struct stat st, st2;
> +        char *i = NULL;
> +        int r;
> +
> +        assert(path);
> +
> +        if (stat(path, &st) < 0)
> +                return NULL;
> +
> +        if (!S_ISBLK(st.st_mode))
> +                return NULL;
> +
> +        asprintf(&i, "/dev/block/%d:%d", major(st.st_rdev), minor(st.st_rdev));
> +
> +        if (!i)
> +                return strdup(path);

I'd really prefer if we would propagate errors properly
here. I.e. change the function to:

static int disk_major_minor(const char *path, char **ret);

Then, return proper error codes, and the newly allocated name in *ret.

> +
> +        r = stat(i, &st2);
> +        if ((r < 0) || (st.st_rdev != st2.st_rdev)) {
> +                free(i);
> +                return strdup(path);
> +        }

Why the second stat() check? I think we can be reasonably sure that
these device nodes will use the right major/minor...

(Also, the extra () seem unnecessary...)

Otherwise looks fine.

Lennart

-- 
Lennart Poettering, Red Hat