storage: Fix FS pool destroy not unmounting its source

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

Posted by Erik Skultety 9 years ago

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@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)
+{
+    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;
+
+ 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 */
-- 
2.10.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 8 years, 12 months ago


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@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 */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 8 years, 12 months ago

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

Well, having the substring "Resolved" in the name IMHO implies that the paths
to be compared should already by resolved in which case the whole point of the
function doesn't make sense, since you'd simply use STREQ. So for this reason I
prefer just ComparePaths. I really wanted it to be called *Equal but sort of
implies the return type boolean which is inapplicable because of the VIR_STRDUP
:(.

> > +{
> > +    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.
>

Well, that was my initial thought, the code would look cleaner, I also liked it
more, but then I thought, most of our compare functions use strcmp for
comparison at some point, so I wanted to follow the convention of returning 0
when equal. However, it wasn't until you suggested it that I grep'ed our code 
and found virReadCompareWWN that works exactly the way I'd favour. So, yeah, I
should have gone with my initial gut feeling, I'll adjust this appropriately,
thanks.

Erik

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/3] storage: Fix reporting an error on an already mounted filesystem
[libvirt] [PATCH 2/3] util: Introduce virFileComparePaths
[libvirt] [PATCH 3/3] storage: Fix checking whether source filesystem is mounted