Libguestfs - Jan 2015 - [PATCH] virt-diff: add additional ignore options

Hi, we needed these changes when we had to build a guest image
compatible with a starting guest image but not backed by it in any way?
We needed some tool to check our progress, comparing original and?
rebuilt (from scratch) images, and virt-diff seemed the best option, but?
we had to soften the comparison to reduce the noise in the output. I
added some options to ignore certain informations when comparing. I
built and tested it, and now for example, one can ignore file permission
with --no-compare-perms, and still see permissions shown when files
differ by other criterias.

added:
--no-compare-xattrs
--no-compare-extra-stats
--no-compare-perms
--no-compare-uids
--no-compare-times

to ignore specified files informations when comparing.

The current strategy to disable comparison on file informations is to
flatten data structure so they return the same 0/NULL value on
comparison and be, in fact, ignored to determine if the files differ.
This patch preserve original information on copies. Comparison is still
made on flattened structures, but preserved structure are used when
showing file informations
---
 diff/diff.c        | 81 +++++++++++++++++++++++++++++++++++++++++++-----------
 diff/virt-diff.pod | 12 ++++++++
 2 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/diff/diff.c b/diff/diff.c
index 6a374af..426184e 100644
--- a/diff/diff.c
+++ b/diff/diff.c
@@ -66,6 +66,11 @@ static int enable_extra_stats = 0;
 static int enable_times = 0;
 static int enable_uids = 0;
 static int enable_xattrs = 0;
+static int no_compare_xattrs = 0;
+static int no_compare_perms = 0;
+static int no_compare_extra_stats = 0;
+static int no_compare_times = 0;
+static int no_compare_uids = 0;
 static int time_t_output = 0;
 static int time_relative = 0; /* 1 = seconds, 2 = days */
 static const char *checksum = NULL;
@@ -125,6 +130,11 @@ usage (int status)
              "  -V|--version         Display version and exit\n"
              "  -x                   Trace libguestfs API calls\n"
              "  --xattrs             Display extended attributes\n"
+             "  --no-compare-xattrs  Do not compare extended
attributes\n"
+             "  --no-compare-extra-stats   Do not compare stats\n"
+             "  --no-compare-perms   Do not compare permissions\n"
+             "  --no-compare-uids    Do not compare uid and gid\n"
+             "  --no-compare-times   Do not compare times\n"
              "For more information, see the manpage %s(1).\n"),
              program_name, program_name, program_name,
              program_name);
@@ -179,6 +189,11 @@ main (int argc, char *argv[])
     { "version", 0, 0, 'V' },
     { "xattr", 0, 0, 0 },
     { "xattrs", 0, 0, 0 },
+    { "no-compare-xattrs", 0, 0, 0 },
+    { "no-compare-perms", 0, 0, 0 },
+    { "no-compare-uids", 0, 0, 0 },
+    { "no-compare-times", 0, 0, 0 },
+    { "no-compare-extra-stats", 0, 0, 0 },
     { 0, 0, 0, 0 }
   };
   struct drv *drvs = NULL;      /* First guest. */
@@ -260,6 +275,16 @@ main (int argc, char *argv[])
       } else if (STREQ (long_options[option_index].name, "xattr") ||
                  STREQ (long_options[option_index].name, "xattrs")) {
         enable_xattrs = 1;
+      } else if (STREQ (long_options[option_index].name,
"no-compare-xattrs")) {
+        no_compare_xattrs = 1;
+      } else if (STREQ (long_options[option_index].name,
"no-compare-perms")) {
+        no_compare_perms = 1;
+      } else if (STREQ (long_options[option_index].name,
"no-compare-uids")) {
+        no_compare_uids = 1;
+      } else if (STREQ (long_options[option_index].name,
"no-compare-times")) {
+        no_compare_times = 1;
+      } else if (STREQ (long_options[option_index].name,
"no-compare-extra-stats")) {
+        no_compare_extra_stats = 1;
       } else {
         fprintf (stderr, _("%s: unknown long option: %s (%d)\n"),
                  program_name, long_options[option_index].name, option_index);
@@ -404,6 +429,8 @@ struct file {
   char *path;
   struct guestfs_statns *stat;
   struct guestfs_xattr_list *xattrs;
+  struct guestfs_statns *stat_orig;
+  struct guestfs_xattr_list *xattrs_orig;
   char *csum;                  /* Checksum. If NULL, use file times and size.
*/
 };
 
@@ -466,6 +493,8 @@ visit_entry (const char *dir, const char *name,
   char *path = NULL, *csum = NULL;
   struct guestfs_statns *stat = NULL;
   struct guestfs_xattr_list *xattrs = NULL;
+  struct guestfs_statns *stat_copy = NULL;
+  struct guestfs_xattr_list *xattrs_copy = NULL;
   size_t i;
 
   path = full_path (dir, name);
@@ -474,15 +503,33 @@ visit_entry (const char *dir, const char *name,
    * free them after we return.
    */
   stat = guestfs_copy_statns (stat_orig);
+  stat_copy = guestfs_copy_statns (stat_orig);
   if (stat == NULL) {
     perror ("guestfs_copy_stat");
     goto error;
   }
+  if (no_compare_perms)
+    stat->st_mode &= 0170000;
+
+  if (no_compare_extra_stats)
+    stat->st_dev = stat->st_ino = stat->st_nlink = stat->st_rdev + 
stat->st_blocks = 0;
+
+  if (no_compare_uids)
+    stat->st_uid = stat->st_gid = 0;
+
+  if (no_compare_times)
+    stat->st_atime_sec = stat->st_mtime_sec = stat->st_ctime_sec +    
stat->st_atime_nsec = stat->st_mtime_nsec = stat->st_ctime_nsec = 0;
+
   xattrs = guestfs_copy_xattr_list (xattrs_orig);
+  xattrs_copy = guestfs_copy_xattr_list (xattrs_orig);
   if (xattrs == NULL) {
     perror ("guestfs_copy_xattr_list");
     goto error;
   }
+  if (no_compare_xattrs)
+    xattrs->len = 0;
 
   if (checksum && is_reg (stat->st_mode)) {
     csum = guestfs_checksum (t->g, checksum, path);
@@ -534,6 +581,8 @@ visit_entry (const char *dir, const char *name,
   t->files[i].stat = stat;
   t->files[i].xattrs = xattrs;
   t->files[i].csum = csum;
+  t->files[i].stat_orig = stat_copy;
+  t->files[i].xattrs_orig = xattrs_copy;
 
   return 0;
 
@@ -778,29 +827,29 @@ output_file (guestfs_h *g, struct file *file)
     filetype = "u";
 
   output_string (filetype);
-  output_int64_perms (file->stat->st_mode & 07777);
+  output_int64_perms (file->stat_orig->st_mode & 07777);
 
-  output_int64_size (file->stat->st_size);
+  output_int64_size (file->stat_orig->st_size);
 
   /* Display extra fields when enabled. */
   if (enable_uids) {
-    output_int64_uid (file->stat->st_uid);
-    output_int64_uid (file->stat->st_gid);
+    output_int64_uid (file->stat_orig->st_uid);
+    output_int64_uid (file->stat_orig->st_gid);
   }
 
   if (enable_times) {
     if (atime)
-      output_int64_time (file->stat->st_atime_sec,
file->stat->st_atime_nsec);
-    output_int64_time (file->stat->st_mtime_sec,
file->stat->st_mtime_nsec);
-    output_int64_time (file->stat->st_ctime_sec,
file->stat->st_ctime_nsec);
+      output_int64_time (file->stat_orig->st_atime_sec,
file->stat_orig->st_atime_nsec);
+    output_int64_time (file->stat_orig->st_mtime_sec,
file->stat_orig->st_mtime_nsec);
+    output_int64_time (file->stat_orig->st_ctime_sec,
file->stat_orig->st_ctime_nsec);
   }
 
   if (enable_extra_stats) {
-    output_int64_dev (file->stat->st_dev);
-    output_int64 (file->stat->st_ino);
-    output_int64 (file->stat->st_nlink);
-    output_int64_dev (file->stat->st_rdev);
-    output_int64 (file->stat->st_blocks);
+    output_int64_dev (file->stat_orig->st_dev);
+    output_int64 (file->stat_orig->st_ino);
+    output_int64 (file->stat_orig->st_nlink);
+    output_int64_dev (file->stat_orig->st_rdev);
+    output_int64 (file->stat_orig->st_blocks);
   }
 
   if (file->csum)
@@ -816,10 +865,10 @@ output_file (guestfs_h *g, struct file *file)
   }
 
   if (enable_xattrs) {
-    for (i = 0; i < file->xattrs->len; ++i) {
-      output_string (file->xattrs->val[i].attrname);
-      output_binary (file->xattrs->val[i].attrval,
-                     file->xattrs->val[i].attrval_len);
+    for (i = 0; i < file->xattrs_orig->len; ++i) {
+      output_string (file->xattrs_orig->val[i].attrname);
+      output_binary (file->xattrs_orig->val[i].attrval,
+                     file->xattrs_orig->val[i].attrval_len);
     }
   }
 }
diff --git a/diff/virt-diff.pod b/diff/virt-diff.pod
index e1d67f3..4ffb33a 100644
--- a/diff/virt-diff.pod
+++ b/diff/virt-diff.pod
@@ -214,6 +214,18 @@ Enable tracing of libguestfs API calls.
 
 Display extended attributes.
 
+=item B<--no-compare-xattrs>
+
+=item B<--no-compare-extra-stats>
+
+=item B<--no-compare-perms>
+
+=item B<--no-compare-uids>
+
+=item B<--no-compare-times>
+
+When comparing, do not consider files different if xattrs, extra-stats,
permissions, uid/gid and times differ, respectively
+
 =back
 
 =head1 NOTE ABOUT CSV FORMAT
-- 
1.9.3

Hi,

In data venerdì 2 gennaio 2015 23:57:43, Gabriele Cerami ha
scritto:
> added:
> --no-compare-xattrs
> --no-compare-extra-stats
> --no-compare-perms
> --no-compare-uids
> --no-compare-times
> 
> to ignore specified files informations when comparing.
> 
> The current strategy to disable comparison on file informations is to
> flatten data structure so they return the same 0/NULL value on
> comparison and be, in fact, ignored to determine if the files differ.
> This patch preserve original information on copies. Comparison is still
> made on flattened structures, but preserved structure are used when
> showing file informations
Hm, I'm a bit dubious whether information that are ignored when
comparing should be shown -- after all, when not enabling some
attribute compared by default (e.g. --atime), that attribute is not
shown if the file differs between images.

I think it would be better to split this patch in two:

a) disabling comparison of some attributes (--no-compare-XXX), by just
   flattening them

b) an extra parameter, or something else, to show not compared
   attributes for files that differ
> diff --git a/diff/diff.c b/diff/diff.c
> index 6a374af..426184e 100644
> --- a/diff/diff.c
> +++ b/diff/diff.c
> @@ -66,6 +66,11 @@ static int enable_extra_stats = 0;
>  static int enable_times = 0;
>  static int enable_uids = 0;
>  static int enable_xattrs = 0;
> +static int no_compare_xattrs = 0;
> +static int no_compare_perms = 0;
> +static int no_compare_extra_stats = 0;
> +static int no_compare_times = 0;
> +static int no_compare_uids = 0;
Instead of "no_compare_XXXX", I'd name them
"compare_XXX" defaulting
to 1, as avoids some mind twisting in expressions like:
  if (!no_compare_perms) ...
>  static int time_t_output = 0;
>  static int time_relative = 0; /* 1 = seconds, 2 = days */
>  static const char *checksum = NULL;
> @@ -125,6 +130,11 @@ usage (int status)
>               "  -V|--version         Display version and exit\n"
>               "  -x                   Trace libguestfs API
calls\n"
>               "  --xattrs             Display extended
attributes\n"
> +             "  --no-compare-xattrs  Do not compare extended
attributes\n"
> +             "  --no-compare-extra-stats   Do not compare
stats\n"
> +             "  --no-compare-perms   Do not compare
permissions\n"
> +             "  --no-compare-uids    Do not compare uid and
gid\n"
> +             "  --no-compare-times   Do not compare times\n"
>               "For more information, see the manpage %s(1).\n"),
Please place them sorted alphabetically (thus after --keys-from-stdin).
>               program_name, program_name, program_name,
>               program_name);
> @@ -179,6 +189,11 @@ main (int argc, char *argv[])
>      { "version", 0, 0, 'V' },
>      { "xattr", 0, 0, 0 },
>      { "xattrs", 0, 0, 0 },
> +    { "no-compare-xattrs", 0, 0, 0 },
> +    { "no-compare-perms", 0, 0, 0 },
> +    { "no-compare-uids", 0, 0, 0 },
> +    { "no-compare-times", 0, 0, 0 },
> +    { "no-compare-extra-stats", 0, 0, 0 },
>      { 0, 0, 0, 0 }
Ditto.
>    };
>    struct drv *drvs = NULL;      /* First guest. */
> @@ -260,6 +275,16 @@ main (int argc, char *argv[])
>        } else if (STREQ (long_options[option_index].name,
"xattr") ||
>                   STREQ (long_options[option_index].name,
"xattrs")) {
>          enable_xattrs = 1;
> +      } else if (STREQ (long_options[option_index].name,
"no-compare-xattrs")) {
> +        no_compare_xattrs = 1;
> +      } else if (STREQ (long_options[option_index].name,
"no-compare-perms")) {
> +        no_compare_perms = 1;
> +      } else if (STREQ (long_options[option_index].name,
"no-compare-uids")) {
> +        no_compare_uids = 1;
> +      } else if (STREQ (long_options[option_index].name,
"no-compare-times")) {
> +        no_compare_times = 1;
> +      } else if (STREQ (long_options[option_index].name,
"no-compare-extra-stats")) {
> +        no_compare_extra_stats = 1;
>        } else {
>          fprintf (stderr, _("%s: unknown long option: %s
(%d)\n"),
>                   program_name, long_options[option_index].name,
option_index);
> @@ -404,6 +429,8 @@ struct file {
>    char *path;
>    struct guestfs_statns *stat;
>    struct guestfs_xattr_list *xattrs;
> +  struct guestfs_statns *stat_orig;
> +  struct guestfs_xattr_list *xattrs_orig;
>    char *csum;                  /* Checksum. If NULL, use file times and
size. */
>  };
>  
> @@ -466,6 +493,8 @@ visit_entry (const char *dir, const char *name,
>    char *path = NULL, *csum = NULL;
>    struct guestfs_statns *stat = NULL;
>    struct guestfs_xattr_list *xattrs = NULL;
> +  struct guestfs_statns *stat_copy = NULL;
> +  struct guestfs_xattr_list *xattrs_copy = NULL;
>    size_t i;
>  
>    path = full_path (dir, name);
> @@ -474,15 +503,33 @@ visit_entry (const char *dir, const char *name,
>     * free them after we return.
>     */
>    stat = guestfs_copy_statns (stat_orig);
> +  stat_copy = guestfs_copy_statns (stat_orig);
>    if (stat == NULL) {
>      perror ("guestfs_copy_stat");
>      goto error;
>    }
> +  if (no_compare_perms)
> +    stat->st_mode &= 0170000;
> +
> +  if (no_compare_extra_stats)
> +    stat->st_dev = stat->st_ino = stat->st_nlink =
stat->st_rdev > +      stat->st_blocks = 0;
> +
> +  if (no_compare_uids)
> +    stat->st_uid = stat->st_gid = 0;
> +
> +  if (no_compare_times)
> +    stat->st_atime_sec = stat->st_mtime_sec = stat->st_ctime_sec
> +      stat->st_atime_nsec = stat->st_mtime_nsec =
stat->st_ctime_nsec = 0;
> +
>    xattrs = guestfs_copy_xattr_list (xattrs_orig);
> +  xattrs_copy = guestfs_copy_xattr_list (xattrs_orig);
>    if (xattrs == NULL) {
>      perror ("guestfs_copy_xattr_list");
>      goto error;
>    }
> +  if (no_compare_xattrs)
> +    xattrs->len = 0;
"len" in _list structs indicates the number of items in the list
itself, so I'd avoid resetting it to 0 otherwise this information is
lost.
Much better to check (no_)compare_xattrs in compare_stats.
Even better, if the xattrs comparison is off, then just avoid copying
xattrs_orig.
>    if (checksum && is_reg (stat->st_mode)) {
>      csum = guestfs_checksum (t->g, checksum, path);
> @@ -534,6 +581,8 @@ visit_entry (const char *dir, const char *name,
>    t->files[i].stat = stat;
>    t->files[i].xattrs = xattrs;
>    t->files[i].csum = csum;
> +  t->files[i].stat_orig = stat_copy;
> +  t->files[i].xattrs_orig = xattrs_copy;
>  
These need to be properly freed in free_tree.
> @@ -778,29 +827,29 @@ output_file (guestfs_h *g, struct file *file)
>      filetype = "u";
>  
>    output_string (filetype);
> -  output_int64_perms (file->stat->st_mode & 07777);
> +  output_int64_perms (file->stat_orig->st_mode & 07777);
>  
> -  output_int64_size (file->stat->st_size);
> +  output_int64_size (file->stat_orig->st_size);
>  
>    /* Display extra fields when enabled. */
>    if (enable_uids) {
> -    output_int64_uid (file->stat->st_uid);
> -    output_int64_uid (file->stat->st_gid);
> +    output_int64_uid (file->stat_orig->st_uid);
> +    output_int64_uid (file->stat_orig->st_gid);
>    }
>  
>    if (enable_times) {
>      if (atime)
> -      output_int64_time (file->stat->st_atime_sec,
file->stat->st_atime_nsec);
> -    output_int64_time (file->stat->st_mtime_sec,
file->stat->st_mtime_nsec);
> -    output_int64_time (file->stat->st_ctime_sec,
file->stat->st_ctime_nsec);
> +      output_int64_time (file->stat_orig->st_atime_sec,
file->stat_orig->st_atime_nsec);
> +    output_int64_time (file->stat_orig->st_mtime_sec,
file->stat_orig->st_mtime_nsec);
> +    output_int64_time (file->stat_orig->st_ctime_sec,
file->stat_orig->st_ctime_nsec);
>    }
>  
>    if (enable_extra_stats) {
> -    output_int64_dev (file->stat->st_dev);
> -    output_int64 (file->stat->st_ino);
> -    output_int64 (file->stat->st_nlink);
> -    output_int64_dev (file->stat->st_rdev);
> -    output_int64 (file->stat->st_blocks);
> +    output_int64_dev (file->stat_orig->st_dev);
> +    output_int64 (file->stat_orig->st_ino);
> +    output_int64 (file->stat_orig->st_nlink);
> +    output_int64_dev (file->stat_orig->st_rdev);
> +    output_int64 (file->stat_orig->st_blocks);
>    }
>  
>    if (file->csum)
> @@ -816,10 +865,10 @@ output_file (guestfs_h *g, struct file *file)
>    }
>  
>    if (enable_xattrs) {
> -    for (i = 0; i < file->xattrs->len; ++i) {
> -      output_string (file->xattrs->val[i].attrname);
> -      output_binary (file->xattrs->val[i].attrval,
> -                     file->xattrs->val[i].attrval_len);
> +    for (i = 0; i < file->xattrs_orig->len; ++i) {
> +      output_string (file->xattrs_orig->val[i].attrname);
> +      output_binary (file->xattrs_orig->val[i].attrval,
> +                     file->xattrs_orig->val[i].attrval_len);
>      }
>    }
>  }
> diff --git a/diff/virt-diff.pod b/diff/virt-diff.pod
> index e1d67f3..4ffb33a 100644
> --- a/diff/virt-diff.pod
> +++ b/diff/virt-diff.pod
> @@ -214,6 +214,18 @@ Enable tracing of libguestfs API calls.
>  
>  Display extended attributes.
>  
> +=item B<--no-compare-xattrs>
> +
> +=item B<--no-compare-extra-stats>
> +
> +=item B<--no-compare-perms>
> +
> +=item B<--no-compare-uids>
> +
> +=item B<--no-compare-times>
> +
> +When comparing, do not consider files different if xattrs, extra-stats,
permissions, uid/gid and times differ, respectively
> +
Just document each one separately, even if with a similar documentation
snippet; something like:

  =item B<--no-compare-xattrs>

  The default is to compare the extended attributes of files. 
  Using this flag ignores the extended attributes when comparing files.

Also, as before, place them sorted alphabetically among the other
options.

Thanks,
-- 
Pino Toscano