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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
