[libvirt] [PATCH 0/3] storage: Fix FS pool destroy not unmounting its source
The issue lies in how we check whether a FS is already mounted or not, we compare the pool's source/target with source/target in the mount's binary output both of which can actually be just symlinks to the same location yet failing our check on the FS being mounted, thus reporting a strange error that the pool's source is already mounted. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1417203 Erik Skultety (3): storage: Fix reporting an error on an already mounted filesystem util: Introduce virFileComparePaths storage: Fix checking whether source filesystem is mounted src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 25 ++++++++++++++-------- src/util/virfile.c | 45 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 2 ++ 4 files changed, 64 insertions(+), 9 deletions(-) -- 2.10.2
When FS pool's source is already mounted on the target location instead of just simply marking the pool as active, thus starting it we fail with an error stating that the source is indeed already mounted on the target. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/storage/storage_backend_fs.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 54bcc57..fe4705b 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -358,14 +358,13 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) if (virStorageBackendFileSystemIsValid(pool) < 0) return -1; + if ((rc = virStorageBackendFileSystemIsMounted(pool)) < 0) + return -1; + /* Short-circuit if already mounted */ - if ((rc = virStorageBackendFileSystemIsMounted(pool)) != 0) { - if (rc == 1) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Target '%s' is already mounted"), - pool->def->target.path); - } - return -1; + if (rc == 1) { + VIR_INFO("Target '%s' is already mounted", pool->def->target.path); + return 0; } if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) -- 2.10.2
On 02/07/2017 09:16 AM, Erik Skultety wrote:
When FS pool's source is already mounted on the target location instead of just simply marking the pool as active, thus starting it we fail with an error stating that the source is indeed already mounted on the target.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/storage/storage_backend_fs.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
ACK John
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
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 */
+ * 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
Right now, we use simple string comparison both on the source paths (mount's output vs pool's source) and the target (mount's mnt_dir vs pool's target). The problem are symlinks and mount indeed returns symlinks in its output, e.g. /dev/mappper/lvm_symlink. The same goes for the pool's source/target, so in order to successfully compare these two replace plain string comparison with virFileComparePaths which will resolve all symlinks and canonicalize the paths prior to comparison. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1417203 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/storage/storage_backend_fs.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index fe4705b..fae1c03 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -301,6 +301,7 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) FILE *mtab; struct mntent ent; char buf[1024]; + int rc1, rc2; if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) { virReportSystemError(errno, @@ -313,8 +314,15 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) goto cleanup; - if (STREQ(ent.mnt_dir, pool->def->target.path) && - STREQ(ent.mnt_fsname, src)) { + /* compare both mount destinations and sources to be sure the mounted + * FS pool is really the one we're looking for + */ + if ((rc1 = virFileComparePaths(ent.mnt_dir, + pool->def->target.path)) < 0 || + (rc2 = virFileComparePaths(ent.mnt_fsname, src)) < 0) + goto cleanup; + + if (rc1 == 0 && rc2 == 0) { ret = 1; goto cleanup; } -- 2.10.2
On 02/07/2017 09:16 AM, Erik Skultety wrote:
Right now, we use simple string comparison both on the source paths (mount's output vs pool's source) and the target (mount's mnt_dir vs pool's target). The problem are symlinks and mount indeed returns symlinks in its output, e.g. /dev/mappper/lvm_symlink. The same goes for the pool's source/target, so in order to successfully compare these two replace plain string comparison with virFileComparePaths which will resolve all symlinks and canonicalize the paths prior to comparison.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1417203
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/storage/storage_backend_fs.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index fe4705b..fae1c03 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -301,6 +301,7 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) FILE *mtab; struct mntent ent; char buf[1024]; + int rc1, rc2;
if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) { virReportSystemError(errno, @@ -313,8 +314,15 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) goto cleanup;
- if (STREQ(ent.mnt_dir, pool->def->target.path) && - STREQ(ent.mnt_fsname, src)) { + /* compare both mount destinations and sources to be sure the mounted + * FS pool is really the one we're looking for + */ + if ((rc1 = virFileComparePaths(ent.mnt_dir, + pool->def->target.path)) < 0 || + (rc2 = virFileComparePaths(ent.mnt_fsname, src)) < 0) + goto cleanup; + + if (rc1 == 0 && rc2 == 0) {
With changes to previous to return 1 on match, then this gets adjusted too... ACK - I think you can alter this appropriately John
ret = 1; goto cleanup; }
On Thu, Feb 09, 2017 at 06:05:41PM -0500, John Ferlan wrote:
On 02/07/2017 09:16 AM, Erik Skultety wrote:
Right now, we use simple string comparison both on the source paths (mount's output vs pool's source) and the target (mount's mnt_dir vs pool's target). The problem are symlinks and mount indeed returns symlinks in its output, e.g. /dev/mappper/lvm_symlink. The same goes for the pool's source/target, so in order to successfully compare these two replace plain string comparison with virFileComparePaths which will resolve all symlinks and canonicalize the paths prior to comparison.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1417203
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/storage/storage_backend_fs.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index fe4705b..fae1c03 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -301,6 +301,7 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) FILE *mtab; struct mntent ent; char buf[1024]; + int rc1, rc2;
if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) { virReportSystemError(errno, @@ -313,8 +314,15 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) goto cleanup;
- if (STREQ(ent.mnt_dir, pool->def->target.path) && - STREQ(ent.mnt_fsname, src)) { + /* compare both mount destinations and sources to be sure the mounted + * FS pool is really the one we're looking for + */ + if ((rc1 = virFileComparePaths(ent.mnt_dir, + pool->def->target.path)) < 0 || + (rc2 = virFileComparePaths(ent.mnt_fsname, src)) < 0) + goto cleanup; + + if (rc1 == 0 && rc2 == 0) {
With changes to previous to return 1 on match, then this gets adjusted too...
ACK - I think you can alter this appropriately
Sure, thanks. Erik
John
ret = 1; goto cleanup; }
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Erik Skultety -
John Ferlan