If the local admin has explicitly declared that a certain
filesystem is to be considered shared, we should treat it as
such.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/util/virfile.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 3268866f8b..45815919d6 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3801,9 +3801,49 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs,
     return NULL;
 }
 
+static bool
+virFileIsSharedFSOverride(const char *path,
+                          char *const *overrides)
+{
+    g_autofree char *dirpath = NULL;
+    char *p = NULL;
+
+    if (!path || path[0] != '/' || !overrides)
+        return false;
+
+    if (g_strv_contains((const char *const *) overrides, path))
+        return true;
+
+    dirpath = g_strdup(path);
+
+    /* Continue until we've scanned the entire path */
+    while (p != dirpath) {
+
+        /* Find the last slash */
+        if ((p = strrchr(dirpath, '/')) == NULL)
+            break;
+
+        /* Truncate the path by overwriting the slash that we've just
+         * found with a null byte. If it is the very first slash in
+         * the path, we need to handle things slightly differently */
+        if (p == dirpath)
+            *(p+1) = '\0';
+        else
+            *p = '\0';
+
+        if (g_strv_contains((const char *const *) overrides, dirpath))
+            return true;
+    }
+
+    return false;
+}
+
 int virFileIsSharedFS(const char *path,
-                      char *const *overrides G_GNUC_UNUSED)
+                      char *const *overrides)
 {
+    if (virFileIsSharedFSOverride(path, overrides))
+        return 1;
+
     return virFileIsSharedFSType(path,
                                  VIR_FILE_SHFS_NFS |
                                  VIR_FILE_SHFS_GFS2 |
-- 
2.44.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
> If the local admin has explicitly declared that a certain
> filesystem is to be considered shared, we should treat it as
> such.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  src/util/virfile.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 3268866f8b..45815919d6 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3801,9 +3801,49 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs,
>      return NULL;
>  }
>  
> +static bool
> +virFileIsSharedFSOverride(const char *path,
> +                          char *const *overrides)
> +{
> +    g_autofree char *dirpath = NULL;
> +    char *p = NULL;
> +
> +    if (!path || path[0] != '/' || !overrides)
> +        return false;

Per my comment on canonicalizing paths only when they're about to be
used.

I think you can also modify the algorithm to avoid the truncate&compare
operations to:


foreach override in overrides:

  pc = canonicalize(path);
  po = canonicalize(override);

  if (STRPREFIX(pc, po))
    return true;

Checking the full prefix on canonicalized paths is IIUC equivalent to
what you do below. (Okay perhaps except the case when user declares a
full to a single file as an exported override).
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, May 09, 2024 at 02:28:15PM GMT, Peter Krempa wrote:
> On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
> > +static bool
> > +virFileIsSharedFSOverride(const char *path,
> > +                          char *const *overrides)
> > +{
> > +    g_autofree char *dirpath = NULL;
> > +    char *p = NULL;
> > +
> > +    if (!path || path[0] != '/' || !overrides)
> > +        return false;
>
> Per my comment on canonicalizing paths only when they're about to be
> used.

Gotcha.

> I think you can also modify the algorithm to avoid the truncate&compare
> operations to:
>
>
> foreach override in overrides:
>
>   pc = canonicalize(path);
>   po = canonicalize(override);
>
>   if (STRPREFIX(pc, po))
>     return true;

I'll give it a try.

> Checking the full prefix on canonicalized paths is IIUC equivalent to
> what you do below. (Okay perhaps except the case when user declares a
> full to a single file as an exported override).

That isn't supposed to work anyway... If the current code allows it
then it will need to be fixed.

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, May 09, 2024 at 06:54:11 -0700, Andrea Bolognani wrote:
> On Thu, May 09, 2024 at 02:28:15PM GMT, Peter Krempa wrote:
> > On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
> > > +static bool
> > > +virFileIsSharedFSOverride(const char *path,
> > > +                          char *const *overrides)
> > > +{
> > > +    g_autofree char *dirpath = NULL;
> > > +    char *p = NULL;
> > > +
> > > +    if (!path || path[0] != '/' || !overrides)
> > > +        return false;
> >
> > Per my comment on canonicalizing paths only when they're about to be
> > used.
> 
> Gotcha.
> 
> > I think you can also modify the algorithm to avoid the truncate&compare
> > operations to:
> >
> >
> > foreach override in overrides:
> >
> >   pc = canonicalize(path);
> >   po = canonicalize(override);
> >
> >   if (STRPREFIX(pc, po))
> >     return true;
> 
> I'll give it a try.
> 
> > Checking the full prefix on canonicalized paths is IIUC equivalent to
> > what you do below. (Okay perhaps except the case when user declares a
> > full to a single file as an exported override).

Okay, so firstly I wrote something else than I've thought.

The algorithm above has a bug if you declare an exported file such
as:

 /path/to/ble

which is a file not a directory, and have  VM with disk pointing to

 /path/to/blesomething

the above code would mark it as shared based on the override. You need
to use the fixed version I've suggested in a reply.

> That isn't supposed to work anyway... If the current code allows it
> then it will need to be fixed.

By going with the meaning that users can mark individual files as
exported, the original impl you've posted does support that:

+    if (g_strv_contains((const char *const *) overrides, path))
+        return true;
+
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, May 09, 2024 at 14:28:15 +0200, Peter Krempa wrote:
> On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:

[...]

> foreach override in overrides:
> 
>   pc = canonicalize(path);
>   po = canonicalize(override);
> 
>   if (STRPREFIX(pc, po))
>     return true;
> 
> Checking the full prefix on canonicalized paths is IIUC equivalent to
> what you do below. (Okay perhaps except the case when user declares a
> full to a single file as an exported override).

To take the above into account we can perhaps do STRSKIP:

   if ((tmp = STRSKIP(pc, po))) {
       if (*tmp == '/' || *tmp == '\0)
          return true;
   }
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org