1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] storage: Fix FS pool destroy not unmounting its source
  4. View patch

[libvirt] [PATCH 3/3] storage: Fix checking whether source filesystem is mounted

Erik Skultety posted 3 patches 9 years ago
[libvirt] [PATCH 3/3] storage: Fix checking whether source filesystem is mounted
Posted by Erik Skultety 9 years ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] storage: Fix checking whether source filesystem is mounted
Posted by John Ferlan 8 years, 12 months ago 

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;
>          }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] storage: Fix checking whether source filesystem is mounted
Posted by Erik Skultety 8 years, 12 months ago 
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

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.