[libvirt] [PATCH 2/3] util: Introduce virFileComparePaths

[John Ferlan]

On 02/07/2017 09:16 AM, Erik Skultety wrote:
> So rather than comparing 2 paths (strings) as they are, which can very
> easily lead to unnecessary errors (e.g. in storage driver) that the paths
> are not the same when in fact they'd be e.g. just symlinks to the same
> location, we should put our best effort into resolving any symlinks and
> canonicalizing the path and only then compare the 2 paths.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virfile.c       | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virfile.h       |  2 ++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8e994c7..06f3737 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1575,6 +1575,7 @@ virFileActivateDirOverride;
>  virFileBindMountDevice;
>  virFileBuildPath;
>  virFileClose;
> +virFileComparePaths;
>  virFileCopyACLs;
>  virFileDeleteTree;
>  virFileDirectFdFlag;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index bf8099e..b261632 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3737,3 +3737,48 @@ virFileCopyACLs(const char *src,
>      virFileFreeACLs(&acl);
>      return ret;
>  }
> +
> +/*
> + * virFileComparePaths:
> + * @p1: source path 1
> + * @p2: source path 2
> + *
> + * Compares two paths for equality. To do so, it first canonicalizes both paths
> + * to resolve all symlinks and discard relative path components. If symlinks
> + * resolution or path canonicalization fails, plain string equality of @p1
> + * and @p2 is performed.
> + *
> + * Returns 0 if the paths are equal, 1 if they're not or -1 in case of an
> + * error.
> + */
> +int
> +virFileComparePaths(const char *p1, const char *p2)

I think this be "virFileCompareResolvedPaths" - change all the places...
 Better representation than just ComparePaths

> +{
> +    int ret = -1;
> +    char *res1, *res2;
> +
> +    res1 = res2 = NULL;
> +
> +    /* Assume p1 and p2 are symlinks, so try to resolve and canonicalize them.
> +     * Canonicalization fails for example on file systems names like 'proc' or
> +     * 'sysfs', since they're no real paths so fallback to plain string
> +     * comparison.
> +     */
> +    ignore_value(virFileResolveLink(p1, &res1));
> +    if (!res1 && VIR_STRDUP(res1, p1) < 0)
> +        goto cleanup;
> +
> +    ignore_value(virFileResolveLink(p2, &res2));
> +    if (!res2 && VIR_STRDUP(res2, p2) < 0)
> +        goto cleanup;
> +
> +    if (STREQ_NULLABLE(res1, res2))
> +        ret = 0;
> +    else
> +        ret = 1;

WHy not return 1 on match, 0 on non match, and -1 on failure that way
all you have to do is "ret = STREQ_NULLABLE(res1, res2);"... If so
adjust the returns comments above too.


ACK w/ adjustments.

> +
> + cleanup:
> +    VIR_FREE(res1);
> +    VIR_FREE(res2);
> +    return ret;
> +}
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 0343acd..5822b29 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -331,4 +331,6 @@ void virFileFreeACLs(void **acl);
>  
>  int virFileCopyACLs(const char *src,
>                      const char *dst);
> +
> +int virFileComparePaths(const char *p1, const char *p2);
>  #endif /* __VIR_FILE_H */
>