[libvirt] [PATCH 3/5] virfile: Introduce virFileMajMinToName

[Michal Privoznik]

On 03/26/2018 11:58 AM, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 07:16:44 +0200, Michal Privoznik wrote:
>> This function can be used to translate MAJ:MIN device pair into
>> /dev/blah name.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virfile.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/util/virfile.h       |  3 +++
>>  3 files changed, 57 insertions(+)
>>
>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index f89e4bd823..62620862a1 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -68,6 +68,12 @@
>>  # include <libdevmapper.h>
>>  #endif
>>  
>> +#ifdef MAJOR_IN_MKDEV
>> +# include <sys/mkdev.h>
>> +#elif MAJOR_IN_SYSMACROS
>> +# include <sys/sysmacros.h>
>> +#endif
>> +
>>  #include "configmake.h"
>>  #include "intprops.h"
>>  #include "viralloc.h"
>> @@ -4317,3 +4323,50 @@ virFileGetMPathTargets(const char *path ATTRIBUTE_UNUSED,
>>      return -1;
>>  }
>>  #endif /* ! WITH_DEVMAPPER */
>> +
>> +
>> +/**
>> + * virFileMajMinToName:
>> + * @device: device (rdev) to translate
>> + * @name: returned name
> 
> I'd prefer if you supply the device identifiers already separated. That
> means that the function in the previous patch should preferrably already
> split them so that we don't need to do it here.

Okay.

> 
>> + *
>> + * For given MAJ:MIN pair (stored in one integer like in st_rdev)
>> + * fetch device name, e.g. 8:0 is translated to "/dev/sda".
>> + * Caller is responsible for freeing @name when no longer needed.
> 
> There is no info on how to treat errors.
> 
>> + *
>> + * Returns 0 on success, -1 otherwise.
> 
> Why isn't the path returned directly?

No reason. It's just that I'm used to functions returning an integer. I
can change it.

> 
>> + */
>> +int
>> +virFileMajMinToName(unsigned long long device,
>> +                    char **name)
>> +{
>> +    struct stat sb;
>> +    char *sysfsPath = NULL;
>> +    char *link = NULL;
>> +    int ret = -1;
>> +
>> +    *name = NULL;
>> +    if (virAsprintf(&sysfsPath, "/sys/dev/block/%u:%u",
>> +                    major(device), minor(device)) < 0)
> 
> So the libdevmapper code is using a similar path to figure out the sysfs
> path, but only for devicemapper devices since it accesses
> /sys/dev/block/%u:%u/dm/name.
> 
> I've also found that '/dev/block/' has the information you might need.
> 
> Is there any particular reason to use sysfs? I'm not really persuaded
> that the directory name in sysfs is good enough so if you can provide
> where you've inspired yourself it will be beneficial.

Okay, I'll switch to /dev/block/... Looks like I took too much
inspiration from other projects O:-)

> 
> Additionally virAsprintf reports libvirt errors but this function does
> not. 
> 
>> +        goto cleanup;
>> +
>> +    if (lstat(sysfsPath, &sb) < 0)
>> +        goto cleanup;
>> +
>> +    if (!S_ISLNK(sb.st_mode)) {
>> +        errno = ENXIO;
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virFileReadLink(sysfsPath, &link) < 0)
>> +        goto cleanup;
>> +
>> +    if (virAsprintf(name, "/dev/%s", last_component(link)) < 0)
>> +        goto cleanup;
> 
> So this is the fishy path. I'd prefer /dev/block since the path already
> points to something in the /dev/ filesystem and isn't conjured up from
> something that looks the same.
> 

Ah, good point. So if I do plain:

virAsprintf(&path, "/dev/block/%u:%u", major(), minor())

I immediately get the correct symlink with both namespace and cgroup
codes know how to deal with. Cool, so this patch might be dropped.

Michal