thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files. [Mar 2020]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2020-Mar-12 14:44 UTC

[Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.

https://bugzilla.redhat.com/show_bug.cgi?id=1811539

Commands including virt-diff which read extended attributes will
sometimes fail on NTFS filesystems that are using system compressed.
The reason is complex, see comment 5 of the bug linked above.

This patch filters out the troublesome xattr.  For justification, see
the comment I added in patch 3.

Patch 1 & 2 are refactoring.

I was on the verge of rewriting the whole file in OCaml, but OCaml
doesn't have access to the relevant system calls so it would have been
a bunch more work and not necessarily any simpler.

Thanks: Yongkui Guo for identifying the bug.

Rich.

Richard W.M. Jones

2020-Mar-12 14:44 UTC

head link

[Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.

The kernel returns xattr names in a slightly peculiar format.  We
parsed this format several times in the code.  Refactoring this
parsing so we only do it in one place.
---
 daemon/xattr.c | 126 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 78 insertions(+), 48 deletions(-)

diff --git a/daemon/xattr.c b/daemon/xattr.c
index 5c9f064ce..482df9af0 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -89,6 +89,35 @@ do_lremovexattr (const char *xattr, const char *path)
   return _removexattr (xattr, path, lremovexattr);
 }
 
+/**
+ * L<listxattr(2)> returns the string C<"foo\0bar\0baz">
of length
+ * C<len>.  (The last string in the list is \0-terminated but the \0
+ * is not included in C<len>).
+ *
+ * This function splits it into a regular list of strings.
+ *
+ * Note the caller must free the returned string list.
+ */
+static char **
+split_attr_names (const char *buf, size_t len)
+{
+  size_t i;
+  DECLARE_STRINGSBUF (ret);
+
+  for (i = 0; i < len; i += strlen (&buf[i]) + 1) {
+    if (add_string (&ret, &buf[i]) == -1) {
+      free_stringsbuf (&ret);
+      return NULL;
+    }
+  }
+  if (end_stringsbuf (&ret) == -1) {
+    free_stringsbuf (&ret);
+    return NULL;
+  }
+
+  return take_stringsbuf (&ret);
+}
+
 static int
 compare_xattrs (const void *vxa1, const void *vxa2)
 {
@@ -106,7 +135,8 @@ getxattrs (const char *path,
 {
   ssize_t len, vlen;
   CLEANUP_FREE char *buf = NULL;
-  size_t i, j;
+  CLEANUP_FREE_STRING_LIST char **names = NULL;
+  size_t i;
   guestfs_int_xattr_list *r = NULL;
 
   buf = _listxattrs (path, listxattr, &len);
@@ -114,18 +144,17 @@ getxattrs (const char *path,
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
+  names = split_attr_names (buf, len);
+  if (names == NULL)
+    goto error;
+
   r = calloc (1, sizeof (*r));
   if (r == NULL) {
     reply_with_perror ("calloc");
     goto error;
   }
 
-  /* What we get from the kernel is a string "foo\0bar\0baz" of
length
-   * len.  First count the strings.
-   */
-  r->guestfs_int_xattr_list_len = 0;
-  for (i = 0; i < (size_t) len; i += strlen (&buf[i]) + 1)
-    r->guestfs_int_xattr_list_len++;
+  r->guestfs_int_xattr_list_len = guestfs_int_count_strings (names);
 
   r->guestfs_int_xattr_list_val      calloc
(r->guestfs_int_xattr_list_len, sizeof (guestfs_int_xattr));
@@ -134,34 +163,34 @@ getxattrs (const char *path,
     goto error;
   }
 
-  for (i = 0, j = 0; i < (size_t) len; i += strlen (&buf[i]) + 1, ++j) {
+  for (i = 0; names[i] != NULL; ++i) {
     CHROOT_IN;
-    vlen = getxattr (path, &buf[i], NULL, 0);
+    vlen = getxattr (path, names[i], NULL, 0);
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr");
+      reply_with_perror ("%s: getxattr", names[i]);
       goto error;
     }
 
     if (vlen > XATTR_SIZE_MAX) {
       /* The next call to getxattr will fail anyway, so ... */
-      reply_with_error ("extended attribute is too large");
+      reply_with_error ("%s: extended attribute is too large",
names[i]);
       goto error;
     }
 
-    r->guestfs_int_xattr_list_val[j].attrname = strdup (&buf[i]);
-    r->guestfs_int_xattr_list_val[j].attrval.attrval_val = malloc (vlen);
-    r->guestfs_int_xattr_list_val[j].attrval.attrval_len = vlen;
+    r->guestfs_int_xattr_list_val[i].attrname = strdup (&buf[i]);
+    r->guestfs_int_xattr_list_val[i].attrval.attrval_val = malloc (vlen);
+    r->guestfs_int_xattr_list_val[i].attrval.attrval_len = vlen;
 
-    if (r->guestfs_int_xattr_list_val[j].attrname == NULL ||
-        r->guestfs_int_xattr_list_val[j].attrval.attrval_val == NULL) {
+    if (r->guestfs_int_xattr_list_val[i].attrname == NULL ||
+        r->guestfs_int_xattr_list_val[i].attrval.attrval_val == NULL) {
       reply_with_perror ("malloc");
       goto error;
     }
 
     CHROOT_IN;
-    vlen = getxattr (path, &buf[i],
-                     r->guestfs_int_xattr_list_val[j].attrval.attrval_val,
+    vlen = getxattr (path, names[i],
+                     r->guestfs_int_xattr_list_val[i].attrval.attrval_val,
                      vlen);
     CHROOT_OUT;
     if (vlen == -1) {
@@ -276,7 +305,7 @@ guestfs_int_xattr_list *
 do_internal_lxattrlist (const char *path, char *const *names)
 {
   guestfs_int_xattr_list *ret = NULL;
-  size_t i, j;
+  size_t i;
   size_t k, m, nr_attrs;
   ssize_t len, vlen;
 
@@ -293,6 +322,7 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     void *newptr;
     CLEANUP_FREE char *pathname = NULL;
     CLEANUP_FREE char *buf = NULL;
+    CLEANUP_FREE_STRING_LIST char **attrnames = NULL;
 
     /* Be careful in this loop about which errors cause the whole call
      * to abort, and which errors allow us to continue processing
@@ -350,12 +380,10 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     if (len == -1)
       continue; /* not fatal */
 
-    /* What we get from the kernel is a string "foo\0bar\0baz" of
length
-     * len.  First count the strings.
-     */
-    nr_attrs = 0;
-    for (i = 0; i < (size_t) len; i += strlen (&buf[i]) + 1)
-      nr_attrs++;
+    attrnames = split_attr_names (buf, len);
+    if (attrnames == NULL)
+      goto error;
+    nr_attrs = guestfs_int_count_strings (attrnames);
 
     newptr        realloc (ret->guestfs_int_xattr_list_val,
@@ -378,36 +406,36 @@ do_internal_lxattrlist (const char *path, char *const
*names)
       entry[m].attrval.attrval_val = NULL;
     }
 
-    for (i = 0, j = 0; i < (size_t) len; i += strlen (&buf[i]) + 1, ++j)
{
+    for (i = 0; attrnames[i] != NULL; ++i) {
       CHROOT_IN;
-      vlen = lgetxattr (pathname, &buf[i], NULL, 0);
+      vlen = lgetxattr (pathname, attrnames[i], NULL, 0);
       CHROOT_OUT;
       if (vlen == -1) {
-        reply_with_perror ("getxattr");
+        reply_with_perror ("%s: getxattr", attrnames[i]);
         goto error;
       }
 
       if (vlen > XATTR_SIZE_MAX) {
-        reply_with_error ("extended attribute is too large");
+        reply_with_error ("%s: extended attribute is too large",
attrnames[i]);
         goto error;
       }
 
-      entry[j+1].attrname = strdup (&buf[i]);
-      entry[j+1].attrval.attrval_val = malloc (vlen);
-      entry[j+1].attrval.attrval_len = vlen;
+      entry[i+1].attrname = strdup (attrnames[i]);
+      entry[i+1].attrval.attrval_val = malloc (vlen);
+      entry[i+1].attrval.attrval_len = vlen;
 
-      if (entry[j+1].attrname == NULL ||
-          entry[j+1].attrval.attrval_val == NULL) {
+      if (entry[i+1].attrname == NULL ||
+          entry[i+1].attrval.attrval_val == NULL) {
         reply_with_perror ("malloc");
         goto error;
       }
 
       CHROOT_IN;
-      vlen = lgetxattr (pathname, &buf[i],
-                        entry[j+1].attrval.attrval_val, vlen);
+      vlen = lgetxattr (pathname, attrnames[i],
+                        entry[i+1].attrval.attrval_val, vlen);
       CHROOT_OUT;
       if (vlen == -1) {
-        reply_with_perror ("getxattr");
+        reply_with_perror ("%s: getxattr", attrnames[i]);
         goto error;
       }
     }
@@ -510,6 +538,7 @@ copy_xattrs (const char *src, const char *dest)
 {
   ssize_t len, vlen, ret, attrval_len = 0;
   CLEANUP_FREE char *buf = NULL, *attrval = NULL;
+  CLEANUP_FREE_STRING_LIST char **names = NULL;
   size_t i;
 
   buf = _listxattrs (src, listxattr, &len);
@@ -517,21 +546,22 @@ copy_xattrs (const char *src, const char *dest)
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
-  /* What we get from the kernel is a string "foo\0bar\0baz" of
length
-   * len.
-   */
-  for (i = 0; i < (size_t) len; i += strlen (&buf[i]) + 1) {
+  names = split_attr_names (buf, len);
+  if (names == NULL)
+    goto error;
+
+  for (i = 0; names[i] != NULL; ++i) {
     CHROOT_IN;
-    vlen = getxattr (src, &buf[i], NULL, 0);
+    vlen = getxattr (src, names[i], NULL, 0);
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr: %s, %s", src, &buf[i]);
+      reply_with_perror ("getxattr: %s, %s", src, names[i]);
       goto error;
     }
 
     if (vlen > XATTR_SIZE_MAX) {
       /* The next call to getxattr will fail anyway, so ... */
-      reply_with_error ("extended attribute is too large");
+      reply_with_error ("%s: extended attribute is too large",
names[i]);
       goto error;
     }
 
@@ -546,18 +576,18 @@ copy_xattrs (const char *src, const char *dest)
     }
 
     CHROOT_IN;
-    vlen = getxattr (src, &buf[i], attrval, vlen);
+    vlen = getxattr (src, names[i], attrval, vlen);
     CHROOT_OUT;
     if (vlen == -1) {
-      reply_with_perror ("getxattr: %s, %s", src, &buf[i]);
+      reply_with_perror ("getxattr: %s, %s", src, names[i]);
       goto error;
     }
 
     CHROOT_IN;
-    ret = setxattr (dest, &buf[i], attrval, vlen, 0);
+    ret = setxattr (dest, names[i], attrval, vlen, 0);
     CHROOT_OUT;
     if (ret == -1) {
-      reply_with_perror ("setxattr: %s, %s", dest, &buf[i]);
+      reply_with_perror ("setxattr: %s, %s", dest, names[i]);
       goto error;
     }
   }
-- 
2.25.0

Richard W.M. Jones

2020-Mar-12 14:44 UTC

head link

[Libguestfs] [PATCH libguestfs 2/3] daemon: Add filter_list utility function.

For filtering lists of strings based on a predicate.
---
 daemon/daemon.h |  1 +
 daemon/utils.c  | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 4d7504b3f..a16953f23 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -74,6 +74,7 @@ extern void free_stringsbuf (struct stringsbuf *sb);
 extern struct stringsbuf split_lines_sb (char *str);
 extern char **split_lines (char *str);
 extern char **empty_list (void);
+extern char **filter_list (int (*p) (const char *), char **strs);
 extern int is_power_of_2 (unsigned long v);
 extern void trim (char *str);
 extern int parse_btrfsvol (const char *desc, mountable_t *mountable);
diff --git a/daemon/utils.c b/daemon/utils.c
index 1cf1b07f6..1fc8d2c80 100644
--- a/daemon/utils.c
+++ b/daemon/utils.c
@@ -482,6 +482,28 @@ empty_list (void)
   return ret.argv;
 }
 
+/**
+ * Filter a list of strings.  Returns a newly allocated list of only
+ * the strings where C<p (str) != 0>.
+ */
+char **
+filter_list (int (*p) (const char *str), char **strs)
+{
+  DECLARE_STRINGSBUF (ret);
+  size_t i;
+
+  for (i = 0; strs[i] != NULL; ++i) {
+    if (p (strs[i]) != 0) {
+      if (add_string (&ret, strs[i]) == -1)
+        return NULL;
+    }
+  }
+  if (end_stringsbuf (&ret) == -1)
+    return NULL;
+
+  return take_stringsbuf (&ret);
+}
+
 /**
  * Skip leading and trailing whitespace, updating the original string
  * in-place.
-- 
2.25.0

Richard W.M. Jones

2020-Mar-12 14:44 UTC

head link

[Libguestfs] [PATCH libguestfs 3/3] daemon: xattr: Filter out user.WofCompressedData from xattrs (RHBZ#1811539).

---
 daemon/xattr.c | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/daemon/xattr.c b/daemon/xattr.c
index 482df9af0..82fb0ebc8 100644
--- a/daemon/xattr.c
+++ b/daemon/xattr.c
@@ -118,6 +118,29 @@ split_attr_names (const char *buf, size_t len)
   return take_stringsbuf (&ret);
 }
 
+/* We hide one extended attribute automatically.  This is used by NTFS
+ * to store the compressed contents of a file when using "CompactOS"
+ * (per-file compression).  I justify this by:
+ *
+ * (1) The attribute is only used internally by NTFS.  The actual file
+ * contents are still available.
+ *
+ * (2) It's probably not valid to copy this attribute when copying the
+ * other attributes of a file.  ntfs-3g-system-compression doesn't
+ * support writing compressed files.
+ *
+ * (3) This file isn't readable by the Linux kernel.  Reading it will
+ * always return -E2BIG (RHBZ#1811539).  So we can't read it even if
+ * we wanted to.
+ *
+ * (4) The Linux kernel itself hides other attributes.
+ */
+static int
+not_hidden_xattr (const char *attrname)
+{
+  return STRNEQ (attrname, "user.WofCompressedData");
+}
+
 static int
 compare_xattrs (const void *vxa1, const void *vxa2)
 {
@@ -135,6 +158,7 @@ getxattrs (const char *path,
 {
   ssize_t len, vlen;
   CLEANUP_FREE char *buf = NULL;
+  CLEANUP_FREE_STRING_LIST char **names_unfiltered = NULL;
   CLEANUP_FREE_STRING_LIST char **names = NULL;
   size_t i;
   guestfs_int_xattr_list *r = NULL;
@@ -144,7 +168,10 @@ getxattrs (const char *path,
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
-  names = split_attr_names (buf, len);
+  names_unfiltered = split_attr_names (buf, len);
+  if (names_unfiltered == NULL)
+    goto error;
+  names = filter_list (not_hidden_xattr, names_unfiltered);
   if (names == NULL)
     goto error;
 
@@ -322,6 +349,7 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     void *newptr;
     CLEANUP_FREE char *pathname = NULL;
     CLEANUP_FREE char *buf = NULL;
+    CLEANUP_FREE_STRING_LIST char **attrnames_unfiltered = NULL;
     CLEANUP_FREE_STRING_LIST char **attrnames = NULL;
 
     /* Be careful in this loop about which errors cause the whole call
@@ -380,7 +408,10 @@ do_internal_lxattrlist (const char *path, char *const
*names)
     if (len == -1)
       continue; /* not fatal */
 
-    attrnames = split_attr_names (buf, len);
+    attrnames_unfiltered = split_attr_names (buf, len);
+    if (attrnames_unfiltered == NULL)
+      goto error;
+    attrnames = filter_list (not_hidden_xattr, attrnames_unfiltered);
     if (attrnames == NULL)
       goto error;
     nr_attrs = guestfs_int_count_strings (attrnames);
@@ -538,6 +569,7 @@ copy_xattrs (const char *src, const char *dest)
 {
   ssize_t len, vlen, ret, attrval_len = 0;
   CLEANUP_FREE char *buf = NULL, *attrval = NULL;
+  CLEANUP_FREE_STRING_LIST char **names_unfiltered = NULL;
   CLEANUP_FREE_STRING_LIST char **names = NULL;
   size_t i;
 
@@ -546,7 +578,10 @@ copy_xattrs (const char *src, const char *dest)
     /* _listxattrs issues reply_with_perror already. */
     goto error;
 
-  names = split_attr_names (buf, len);
+  names_unfiltered = split_attr_names (buf, len);
+  if (names_unfiltered == NULL)
+    goto error;
+  names = filter_list (not_hidden_xattr, names_unfiltered);
   if (names == NULL)
     goto error;
 
-- 
2.25.0

Richard W.M. Jones

2020-Mar-12 22:51 UTC

head link

Re: [Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.

On Thu, Mar 12, 2020 at 02:44:46PM +0000, Richard W.M. Jones
wrote:> -    r->guestfs_int_xattr_list_val[j].attrname = strdup (&buf[i]);
> -    r->guestfs_int_xattr_list_val[j].attrval.attrval_val = malloc
(vlen);
> -    r->guestfs_int_xattr_list_val[j].attrval.attrval_len = vlen;
> +    r->guestfs_int_xattr_list_val[i].attrname = strdup (&buf[i]);
I guess our testing doesn't cover this function, because
it would certainly have crashed here.  &buf[i] should be names[i].
I have updated my local copy.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Pino Toscano

2020-Mar-13 13:20 UTC

head link

Re: [Libguestfs] [PATCH libguestfs 2/3] daemon: Add filter_list utility function.

On Thursday, 12 March 2020 15:44:47 CET Richard W.M. Jones
wrote:> For filtering lists of strings based on a predicate.
> ---
>  daemon/daemon.h |  1 +
>  daemon/utils.c  | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 4d7504b3f..a16953f23 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -74,6 +74,7 @@ extern void free_stringsbuf (struct stringsbuf *sb);
>  extern struct stringsbuf split_lines_sb (char *str);
>  extern char **split_lines (char *str);
>  extern char **empty_list (void);
> +extern char **filter_list (int (*p) (const char *), char **strs);
>  extern int is_power_of_2 (unsigned long v);
>  extern void trim (char *str);
>  extern int parse_btrfsvol (const char *desc, mountable_t *mountable);
> diff --git a/daemon/utils.c b/daemon/utils.c
> index 1cf1b07f6..1fc8d2c80 100644
> --- a/daemon/utils.c
> +++ b/daemon/utils.c
> @@ -482,6 +482,28 @@ empty_list (void)
>    return ret.argv;
>  }
>  
> +/**
> + * Filter a list of strings.  Returns a newly allocated list of only
> + * the strings where C<p (str) != 0>.
Maybe use bool as return value instead?
> + */
> +char **
> +filter_list (int (*p) (const char *str), char **strs)
> +{
> +  DECLARE_STRINGSBUF (ret);
Declare this as CLEANUP_FREE_STRINGSBUF, so the stringsbuf is properly
cleaned on failure. take_stringsbuf at the return is fine, as it will
hand over the resources to the caller.

-- 
Pino Toscano

Pino Toscano

2020-Mar-13 13:28 UTC

head link

Re: [Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.

On Thursday, 12 March 2020 15:44:46 CET Richard W.M. Jones
wrote:> +static char **
> +split_attr_names (const char *buf, size_t len)
> +{
> +  size_t i;
> +  DECLARE_STRINGSBUF (ret);
Declare this as CLEANUP_FREE_STRINGSBUF, so there is no need for the
manual calls to free_stringsbuf on failure. take_stringsbuf at the
return is fine, as it will hand over the resources to the caller.
>      CHROOT_OUT;
>      if (vlen == -1) {
> -      reply_with_perror ("getxattr");
> +      reply_with_perror ("%s: getxattr", names[i]);
IIRC "what: details" is more common, so "getxattr: %s".
(Same for the
other ocurrences.)
>        goto error;
>      }
>  
>      if (vlen > XATTR_SIZE_MAX) {
>        /* The next call to getxattr will fail anyway, so ... */
> -      reply_with_error ("extended attribute is too large");
> +      reply_with_error ("%s: extended attribute is too large",
names[i]);
>        goto error;
>      }
>  
> -    r->guestfs_int_xattr_list_val[j].attrname = strdup (&buf[i]);
> -    r->guestfs_int_xattr_list_val[j].attrval.attrval_val = malloc
(vlen);
> -    r->guestfs_int_xattr_list_val[j].attrval.attrval_len = vlen;
> +    r->guestfs_int_xattr_list_val[i].attrname = strdup (&buf[i]);
names[i]

[you already fixed this, I just amended this message before sending it,
as I started to write it yesterday]
> @@ -378,36 +406,36 @@ do_internal_lxattrlist (const char *path, char *const
*names)
>        entry[m].attrval.attrval_val = NULL;
>      }
>  
> -    for (i = 0, j = 0; i < (size_t) len; i += strlen (&buf[i]) + 1,
++j) {
> +    for (i = 0; attrnames[i] != NULL; ++i) {
Considering we have nr_attrs at this point, we can use it.

-- 
Pino Toscano

-- 
Pino Toscano

Pino Toscano

2020-Mar-13 14:01 UTC

head link

Re: [Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.

On Thursday, 12 March 2020 15:44:45 CET Richard W.M. Jones
wrote:> https://bugzilla.redhat.com/show_bug.cgi?id=1811539
> 
> Commands including virt-diff which read extended attributes will
> sometimes fail on NTFS filesystems that are using system compressed.
> The reason is complex, see comment 5 of the bug linked above.
> 
> This patch filters out the troublesome xattr.  For justification, see
> the comment I added in patch 3.
The idea is fine, however I'm slightly concerned about the increased
memory usage due to the split + filtering done.

Now that I took a look at the code again (wow, last time I touched it
was more than 5 years ago), an alternative approach could be:
- refactor do_internal_lxattrlist to use _listxattrs
- edit/replace in-place the buffer to-be-returned by _listxattrs
  filtering out the names we do not want
This way the rest of the existing code needs almost no changes.

-- 
Pino Toscano

Richard W.M. Jones

2020-Mar-16 12:20 UTC

head link

Re: [Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.

On Fri, Mar 13, 2020 at 03:01:00PM +0100, Pino Toscano
wrote:> On Thursday, 12 March 2020 15:44:45 CET Richard W.M. Jones wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1811539
> > 
> > Commands including virt-diff which read extended attributes will
> > sometimes fail on NTFS filesystems that are using system compressed.
> > The reason is complex, see comment 5 of the bug linked above.
> > 
> > This patch filters out the troublesome xattr.  For justification, see
> > the comment I added in patch 3.
> 
> The idea is fine, however I'm slightly concerned about the increased
> memory usage due to the split + filtering done.
> 
> Now that I took a look at the code again (wow, last time I touched it
> was more than 5 years ago), an alternative approach could be:
> - refactor do_internal_lxattrlist to use _listxattrs
This isn't totally straightforward because the functions handle errors
in slightly different ways (of course we could add a very specific
bool parameter for that case, but ...)
> - edit/replace in-place the buffer to-be-returned by _listxattrs
>   filtering out the names we do not want
> This way the rest of the existing code needs almost no changes.
So my alternate optimization would be to do the list filtering as per
the patch I posted, but not strdup the strings themselves.  It
achieves mainly the same end as what you're saying but is a lot safer
and clearer than trying to edit in place the weird "multi-string" that
the kernel returns.  I'll try that.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Seemingly Similar Threads

Search for more maybe matching threads

Libguestfs - Mar 2020 - Re: [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.

[Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.

[Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.

[Libguestfs] [PATCH libguestfs 2/3] daemon: Add filter_list utility function.

[Libguestfs] [PATCH libguestfs 3/3] daemon: xattr: Filter out user.WofCompressedData from xattrs (RHBZ#1811539).

Re: [Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.

Re: [Libguestfs] [PATCH libguestfs 2/3] daemon: Add filter_list utility function.

Re: [Libguestfs] [PATCH libguestfs 1/3] daemon: xattr: Refactor code which splits attr names from the kernel.

Re: [Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.

Re: [Libguestfs] [PATCH libguestfs 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.

Seemingly Similar Threads