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

[Harald Hoyer]

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?

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

just copied the scheme of disk_description() and disk_mount_point() :-)

> 
>> +
>> +        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.
> 
> (Also, the extra () seem unnecessary...)
> 
> Otherwise looks fine.
> 
> Lennart
>