5:25 a.m.
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