1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/4] Couple of virFileIsShared* fixes
  4. View patch

[libvirt] [PATCH 4/4] virfile: Rework virFileIsSharedFixFUSE

Michal Privoznik posted 4 patches 5 years, 11 months ago
[libvirt] [PATCH 4/4] virfile: Rework virFileIsSharedFixFUSE
Posted by Michal Privoznik 5 years, 11 months ago 
There are couple of things wrong with the current implementation.
The first one is that in the first loop the code tries to build a
list of fuse.glusterfs mount points. Well, since the strings are
allocated in a temporary buffer and are not duplicated this
results in wrong decision made later in the code.

The second problem is that the code does not take into account
subtree mounts. For instance, if there's a fuse.gluster mounted
at /some/path and another FS mounted at /some/path/subdir the
code would not recognize this subdir mount.

Reported-by: Han Han <hhan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virfile.c            | 58 +++++++++++++----------------------
 tests/virfiledata/mounts3.txt |  2 ++
 tests/virfiletest.c           |  2 ++
 3 files changed, 26 insertions(+), 36 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 666d703f99..19f504cc6d 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3468,18 +3468,14 @@ static int
 virFileIsSharedFixFUSE(const char *path,
                        long *f_type)
 {
-    char *dirpath = NULL;
-    const char **mounts = NULL;
-    size_t nmounts = 0;
-    char *p;
     FILE *f = NULL;
     struct mntent mb;
     char mntbuf[1024];
+    char *mntDir = NULL;
+    char *mntType = NULL;
+    size_t maxMatching = 0;
     int ret = -1;
 
-    if (VIR_STRDUP(dirpath, path) < 0)
-        return -1;
-
     if (!(f = setmntent(PROC_MOUNTS, "r"))) {
         virReportSystemError(errno,
                              _("Unable to open %s"),
@@ -3488,43 +3484,33 @@ virFileIsSharedFixFUSE(const char *path,
     }
 
     while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) {
-        if (STRNEQ("fuse.glusterfs", mb.mnt_type))
+        const char *p;
+        size_t len = strlen(mb.mnt_dir);
+
+        if (!(p = STRSKIP(path, mb.mnt_dir)))
             continue;
 
-        if (VIR_APPEND_ELEMENT_COPY(mounts, nmounts, mb.mnt_dir) < 0)
-            goto cleanup;
-    }
-
-    /* Add NULL sentinel so that this is a virStringList */
-    if (VIR_REALLOC_N(mounts, nmounts + 1) < 0)
-        goto cleanup;
-    mounts[nmounts] = NULL;
-
-    do {
-        if ((p = strrchr(dirpath, '/')) == NULL) {
-            virReportSystemError(EINVAL,
-                                 _("Invalid relative path '%s'"), path);
-            goto cleanup;
+        if (len > maxMatching) {
+            len = maxMatching;
+            VIR_FREE(mntType);
+            VIR_FREE(mntDir);
+            if (VIR_STRDUP(mntDir, mb.mnt_dir) < 0 ||
+                VIR_STRDUP(mntType, mb.mnt_type) < 0)
+                goto cleanup;
         }
+    }
 
-        if (p == dirpath)
-            *(p+1) = '\0';
-        else
-            *p = '\0';
-
-        if (virStringListHasString(mounts, dirpath)) {
-            VIR_DEBUG("Found gluster FUSE mountpoint=%s for path=%s. "
-                      "Fixing shared FS type", dirpath, path);
-            *f_type = GFS2_MAGIC;
-            break;
-        }
-    } while (p != dirpath);
+    if (STREQ_NULLABLE(mntType, "fuse.glusterfs")) {
+        VIR_DEBUG("Found gluster FUSE mountpoint=%s for path=%s. "
+                  "Fixing shared FS type", mntDir, path);
+        *f_type = GFS2_MAGIC;
+    }
 
     ret = 0;
  cleanup:
+    VIR_FREE(mntType);
+    VIR_FREE(mntDir);
     endmntent(f);
-    VIR_FREE(mounts);
-    VIR_FREE(dirpath);
     return ret;
 }
 
diff --git a/tests/virfiledata/mounts3.txt b/tests/virfiledata/mounts3.txt
index 226f67dc00..134c6e8f81 100644
--- a/tests/virfiledata/mounts3.txt
+++ b/tests/virfiledata/mounts3.txt
@@ -31,3 +31,5 @@ hugetlbfs /hugepages2M hugetlbfs rw,relatime,mode=1777,pagesize=2M 0 0
 none /run/user/1000 tmpfs rw,relatime,mode=700,uid=1000 0 0
 host:/nfs /nfs nfs4 rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp6,timeo=600,retrans=2,sec=sys,clientaddr=::,local_lock=none,addr=:: 0 0
 dev /nfs/blah devtmpfs rw,nosuid,relatime,size=10240k,nr_inodes=4093060,mode=755 0 0
+host:/gv0 /gluster fuse.glusterfs rw 0 0
+root@host:/tmp/mkdir /gluster/sshfs fuse.sshfs rw 0 0
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index 42d918aecc..d5102b1cc4 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -454,6 +454,8 @@ mymain(void)
     DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts2.txt", "/run/user/501/gvfs/some/file", false);
     DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/nfs/file", true);
     DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/nfs/blah", false);
+    DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gluster/file", true);
+    DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gluster/sshfs/file", false);
 
     return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS;
 }
-- 
2.18.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] virfile: Rework virFileIsSharedFixFUSE
Posted by Jiri Denemark 5 years, 11 months ago 
On Tue, Oct 09, 2018 at 15:48:47 +0200, Michal Privoznik wrote:
> There are couple of things wrong with the current implementation.
> The first one is that in the first loop the code tries to build a
> list of fuse.glusterfs mount points. Well, since the strings are
> allocated in a temporary buffer and are not duplicated this
> results in wrong decision made later in the code.
> 
> The second problem is that the code does not take into account
> subtree mounts. For instance, if there's a fuse.gluster mounted
> at /some/path and another FS mounted at /some/path/subdir the
> code would not recognize this subdir mount.
> 
> Reported-by: Han Han <hhan@redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/virfile.c            | 58 +++++++++++++----------------------
>  tests/virfiledata/mounts3.txt |  2 ++
>  tests/virfiletest.c           |  2 ++
>  3 files changed, 26 insertions(+), 36 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 666d703f99..19f504cc6d 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3468,18 +3468,14 @@ static int
>  virFileIsSharedFixFUSE(const char *path,
>                         long *f_type)
>  {
> -    char *dirpath = NULL;
> -    const char **mounts = NULL;
> -    size_t nmounts = 0;
> -    char *p;
>      FILE *f = NULL;
>      struct mntent mb;
>      char mntbuf[1024];
> +    char *mntDir = NULL;
> +    char *mntType = NULL;
> +    size_t maxMatching = 0;
>      int ret = -1;
>  
> -    if (VIR_STRDUP(dirpath, path) < 0)
> -        return -1;
> -
>      if (!(f = setmntent(PROC_MOUNTS, "r"))) {
>          virReportSystemError(errno,
>                               _("Unable to open %s"),
> @@ -3488,43 +3484,33 @@ virFileIsSharedFixFUSE(const char *path,
>      }
>  
>      while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) {
> -        if (STRNEQ("fuse.glusterfs", mb.mnt_type))
> +        const char *p;
> +        size_t len = strlen(mb.mnt_dir);
> +
> +        if (!(p = STRSKIP(path, mb.mnt_dir)))
>              continue;

I think you wanted to do something clever with the p pointer here to
avoid false positives on, e.g., /mnt/ble when searching for /mnt/bleak
path. Otherwise you could have just use STRPREFIX().

The rest of the patch looks OK.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] virfile: Rework virFileIsSharedFixFUSE
Posted by Michal Privoznik 5 years, 11 months ago 
On 10/10/2018 01:08 PM, Jiri Denemark wrote:
> On Tue, Oct 09, 2018 at 15:48:47 +0200, Michal Privoznik wrote:
>> There are couple of things wrong with the current implementation.
>> The first one is that in the first loop the code tries to build a
>> list of fuse.glusterfs mount points. Well, since the strings are
>> allocated in a temporary buffer and are not duplicated this
>> results in wrong decision made later in the code.
>>
>> The second problem is that the code does not take into account
>> subtree mounts. For instance, if there's a fuse.gluster mounted
>> at /some/path and another FS mounted at /some/path/subdir the
>> code would not recognize this subdir mount.
>>
>> Reported-by: Han Han <hhan@redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/util/virfile.c            | 58 +++++++++++++----------------------
>>  tests/virfiledata/mounts3.txt |  2 ++
>>  tests/virfiletest.c           |  2 ++
>>  3 files changed, 26 insertions(+), 36 deletions(-)
>>
>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index 666d703f99..19f504cc6d 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -3468,18 +3468,14 @@ static int
>>  virFileIsSharedFixFUSE(const char *path,
>>                         long *f_type)
>>  {
>> -    char *dirpath = NULL;
>> -    const char **mounts = NULL;
>> -    size_t nmounts = 0;
>> -    char *p;
>>      FILE *f = NULL;
>>      struct mntent mb;
>>      char mntbuf[1024];
>> +    char *mntDir = NULL;
>> +    char *mntType = NULL;
>> +    size_t maxMatching = 0;
>>      int ret = -1;
>>  
>> -    if (VIR_STRDUP(dirpath, path) < 0)
>> -        return -1;
>> -
>>      if (!(f = setmntent(PROC_MOUNTS, "r"))) {
>>          virReportSystemError(errno,
>>                               _("Unable to open %s"),
>> @@ -3488,43 +3484,33 @@ virFileIsSharedFixFUSE(const char *path,
>>      }
>>  
>>      while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) {
>> -        if (STRNEQ("fuse.glusterfs", mb.mnt_type))
>> +        const char *p;
>> +        size_t len = strlen(mb.mnt_dir);
>> +
>> +        if (!(p = STRSKIP(path, mb.mnt_dir)))
>>              continue;
> 
> I think you wanted to do something clever with the p pointer here to
> avoid false positives on, e.g., /mnt/ble when searching for /mnt/bleak
> path. Otherwise you could have just use STRPREFIX().
> 
> The rest of the patch looks OK.


Oh right. I wanted to do this:

diff --git i/src/util/virfile.c w/src/util/virfile.c
index 19f504cc6d..03e14c757f 100644
--- i/src/util/virfile.c
+++ w/src/util/virfile.c
@@ -3490,6 +3490,9 @@ virFileIsSharedFixFUSE(const char *path,
         if (!(p = STRSKIP(path, mb.mnt_dir)))
             continue;

+        if (*p != '/')
+            continue;
+
         if (len > maxMatching) {
             len = maxMatching;
             VIR_FREE(mntType);


Michal

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

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.