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

[Peter Krempa]

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.

> + *
> + * 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?

> + */
> +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.

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.

> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(link);
> +    VIR_FREE(sysfsPath);
> +    return ret;
> +}
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 0db8bf0b99..390940dc98 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -362,4 +362,7 @@ int virFileGetMPathTargets(const char *path,
>                             unsigned long long **devs,
>                             size_t *ndevs);
>  
> +int virFileMajMinToName(unsigned long long device,
> +                        char **name);
> +
>  #endif /* __VIR_FILE_H */
> -- 
> 2.16.1
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180326/87b5c39d/attachment-0001.sig>