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

[Pino Toscano]

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