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

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

On 05 Jan, Pino Toscano wrote:
> 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
The first draft of this patch disallowed ignoring an information (e.g.
xattrs) and enabling it in output strings. But for example permissions
are always shown in diff, and I wanted definitely to check if permission
were creating a compatibility problem with the original image we wanted
to reproduce.
I can see no harm in showing all the informations even if we are
ignoring them. I think we wanted to see all them anyway  to get
confirmation that things worked they way we intended.
> 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) ...
So what the argument should look like when invoked ?
virt-diff --compare-xattrs=0 ? What you suggest should be the name of
the argument to show all the compare=False informations ?
> 
> "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.
> 
what is the problem if the information is lost ? there's the same
information in xattrs_orig. Anyway, the second proposal is feasible but
not similar to the strategies for others "ignore flags" (that is
flatten)
and for example cannot be done with extra-stats. I can do it, but I did
not considered it because it moves the logic for "ignoring" away from
the other checks.
Regarding the third proposal guestfs_compare_xattr_list and
guestfs_compare_xattr expect the struct not to be NULL, so if I call
those functions, I cannot pass a NULL pointer and setting len to 0 was
the only way to make the function return the value I wanted (0 - not
diff)



Thanks for the suggestion, I'll submit the corrections as soon as I can.

In data martedì 6 gennaio 2015 18:50:52, Gabriele Cerami ha
scritto:
> On 05 Jan, Pino Toscano wrote:
> > 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
> 
> The first draft of this patch disallowed ignoring an information (e.g.
> xattrs) and enabling it in output strings. But for example permissions
> are always shown in diff, and I wanted definitely to check if permission
> were creating a compatibility problem with the original image we wanted
> to reproduce.
Permissions are always shown because they are always compared.
> I can see no harm in showing all the informations even if we are
> ignoring them. I think we wanted to see all them anyway  to get
> confirmation that things worked they way we intended.
> 
> > 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) ...
> 
> So what the argument should look like when invoked ?
> virt-diff --compare-xattrs=0 ? What you suggest should be the name of
> the argument to show all the compare=False informations ?
The note was just about the naming of the internal variables, not for
the command line options.
> > "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.
> > 
> 
> what is the problem if the information is lost ? there's the same
> information in xattrs_orig.
These structs are created by the RPC protocol code (in src/, see the
generated guestfs_protocol.x, guestfs_protocol.c, and structs-free.c
for the cleanup routines for the structs).
I don't know whether the implementation (e.g. xdr_free) relies on the
_len values (most probably doesn't), but I would still avoid changing
them.
> Anyway, the second proposal is feasible but
> not similar to the strategies for others "ignore flags" (that is
flatten)
> and for example cannot be done with extra-stats. I can do it, but I did
> not considered it because it moves the logic for "ignoring" away
from
> the other checks.
Ignored flags in stat structs are flattened because they are passed to
an (auto-generated) function to compare them, to avoid redoing all
the comparisons manually.
> Regarding the third proposal guestfs_compare_xattr_list and
> guestfs_compare_xattr expect the struct not to be NULL, so if I call
> those functions, I cannot pass a NULL pointer and setting len to 0 was
> the only way to make the function return the value I wanted (0 - not
> diff)
  assert ((file1->xattrs != NULL && file2->xattrs != NULL) ||
          (file1->xattrs == NULL && file2->xattrs == NULL));
  if (file1->xattrs != NULL && file2->xattrs != NULL) {
    r = guestfs_compare_xattr_list (file1->xattrs, file2->xattrs);
    if (r != 0)
      return r;
  }

Since we would set xattrs only when comparing them, and for every file
in every image, either both are null or aren't.

Thanks,
-- 
Pino Toscano