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

[Lennart Poettering]

On Fri, 29.05.15 17:11, Harald Hoyer (harald at redhat.com) wrote:

> Am 29.05.2015 um 17:04 schrieb 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);
> 
> ret as the last or the first argument?

Usually we put that last.

(except for object constructors, where we put it first since object
methods always get their object specified as first arg...)

> > Then, return proper error codes, and the newly allocated name in *ret.
> 
> just copied the scheme of disk_description() and disk_mount_point()
> :-)

Yeah, I guess that code is just old...

> >> +
> >> +        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...
> 
> Yes, what if it is missing? Someone removed it or played games :)
> Sure, can remove the rdev check.

Well, I#d prefer to avoid additional stat()s unless we have a really
strong reason to have them in place.

Lennart

-- 
Lennart Poettering, Red Hat