[libvirt] [PATCH 2/5] utils: Introduce virFileGetMPathTargets

[Peter Krempa]

On Mon, Mar 26, 2018 at 07:16:43 +0200, Michal Privoznik wrote:
> This helper fetches targets for multipath devices.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virfile.c       | 90 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virfile.h       |  4 +++
>  3 files changed, 95 insertions(+)

As I've pointed in the conversation regarding patch 1/5, this should be
put into a separate file, so that it's obvious what needs to be removed
if this functionality will need to be put into the storage driver.

Additionally it then will be justified to use a virOnce there to set the
logging function for libdevmapper.

> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 5e9bd2007a..f89e4bd823 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c

> @@ -4227,3 +4231,89 @@ virFileWaitForExists(const char *path,
>  
>      return 0;
>  }
> +
> +
> +#ifdef WITH_DEVMAPPER
> +/**
> + * virFileGetMPathTargets:
> + * @path: multipath device
> + * @devs: returned array of st_rdevs of @path targets
> + * @ndevs: size of @devs and @devNames

There's no 'devNames' parameter.

> + *
> + * For given @path figure out its targets, and store them in
> + * @devs. Items from @devs are meant to be used in
> + * minor() and major() to receive device MAJ:MIN pairs.
> + *
> + * If @path is not a multipath device, @ndevs is set to 0 and
> + * success is returned.
> + *
> + * If we don't have permissions to talk to kernel, -1 is returned
> + * and errno is set to EBADF.

You need to document that this function does not use libvirt errors.

> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> +int
> +virFileGetMPathTargets(const char *path,
> +                       unsigned long long **devs,

struct 'dm_deps' uses uint64_t for this.

> +                       size_t *ndevs)
> +{
> +    struct dm_deps *deps;
> +    struct dm_task *dmt;
> +    struct dm_info info;
> +    size_t i;
> +    int ret = -1;
> +
> +    *ndevs = 0;
> +
> +    if (!(dmt = dm_task_create(DM_DEVICE_DEPS)))
> +        goto cleanup;
> +
> +    if (!dm_task_set_name(dmt, path)) {
> +        if (errno == ENOENT) {
> +            /* It's okay, @path is not managed by devmapper =>
> +             * not a multipath device. */
> +            ret = 0;
> +        }
> +        goto cleanup;
> +    }
> +
> +    dm_task_no_open_count(dmt);
> +
> +    if (!dm_task_run(dmt))
> +        goto cleanup;
> +
> +    if (!dm_task_get_info(dmt, &info))
> +        goto cleanup;
> +
> +    if (!info.exists) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    if (!(deps = dm_task_get_deps(dmt)))
> +        goto cleanup;
> +
> +    if (VIR_ALLOC_N(*devs, deps->count) < 0)

VIR_ALLOC_N_QUIET, since we don't do libvirt errors.

> +        goto cleanup;
> +    *ndevs = deps->count;
> +
> +    for (i = 0; i < deps->count; i++)
> +        (*devs)[i] = deps->device[i];
> +
> +    ret = 0;
> + cleanup:
> +    dm_task_destroy(dmt);
> +    return ret;
> +}


Okay this code is stol^W inspired by the 'dmsetup deps' functionality.
You might want to point that fact out, since it took some time to find
it.

The code looks good, but since it requires the virOnce stuff, v2 will be
needed.
-------------- 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/5c7c4ed1/attachment-0001.sig>