[Libguestfs] [PATCH] virt-diff: add additional ignore options
Pino Toscano
ptoscano at redhat.com
Mon Jan 5 15:21:17 UTC 2015
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
More information about the Libguestfs
mailing list