Re: [Libguestfs] [PATCH V3] virt-diff: add new virt-diff tool

On Mon, Aug 20, 2012 at 04:22:23PM +0800, Wanlong Gao wrote: > add new virt-diff tool > > Signed-off-by: Wanlong Gao <gaowanlong(a)cn.fujitsu.com&gt; > --- > cat/Makefile.am | 20 ++- > cat/virt-diff.c | 525 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > po/POTFILES | 1 + > 3 files changed, 545 insertions(+), 1 deletion(-) > create mode 100644 cat/virt-diff.c > > diff --git a/cat/Makefile.am b/cat/Makefile.am > index f7c763a..5f6a986 100644 > --- a/cat/Makefile.am > +++ b/cat/Makefile.am > @@ -27,7 +27,7 @@ EXTRA_DIST = \ > > CLEANFILES = stamp-virt-cat.pod stamp-virt-ls.pod stamp-virt-filesystems.pod This program has to have a manual page.

> + exit (EXIT_FAILURE); > + > + if (roots[0] == NULL) { > + fprintf (stderr, > + _("%s: no operating system was found on this disk\n" > + "\n" > + "If using guestfish '-i' option, remove this option and instead\n" > + "use the commands 'run' followed by 'list-filesystems'.\n" > + "You can then mount filesystems you want by hand using the\n" > + "'mount' or 'mount-ro' command.\n" > + "\n" > + "If using guestmount '-i', remove this option and choose the\n" > + "filesystem(s) you want to see by manually adding '-m' option(s).\n" > + "Use 'virt-filesystems' to see what filesystems are available.\n" > + "\n" > + "If using other virt tools, this disk image won't work\n" > + "with these tools. Use the guestfish equivalent commands\n" > + "(see the virt tool manual page).\n"), > + program_name); > + free_strings (roots); > + exit (EXIT_FAILURE); > + } This long generic text copied from guestfish isn't appropriate. Just reduce it to the first line ("no operating system was found ...").

> + if (roots[1] != NULL) { > + fprintf (stderr, > + _("%s: multi-boot operating systems are not supported\n" > + "\n" > + "If using guestfish '-i' option, remove this option and instead\n" > + "use the commands 'run' followed by 'list-filesystems'.\n" > + "You can then mount filesystems you want by hand using the\n" > + "'mount' or 'mount-ro' command.\n" > + "\n" > + "If using guestmount '-i', remove this option and choose the\n" > + "filesystem(s) you want to see by manually adding '-m' option(s).\n" > + "Use 'virt-filesystems' to see what filesystems are available.\n" > + "\n" > + "If using other virt tools, multi-boot operating systems won't work\n" > + "with these tools. Use the guestfish equivalent commands\n" > + "(see the virt tool manual page).\n"), > + program_name); > + free_strings (roots); > + exit (EXIT_FAILURE); > + } And the same here.

> + root = roots[0]; > + free (roots); > + > + diff_inspect_mount_root (g, root); > +} > + > +static void > +free_drive (struct drv *drv) > +{ > + if (!drv) return; > + > + free (drv->device); > + free (drv); > +} > + > +int > +main (int argc, char *argv[]) > +{ > + /* set global program name that is not polluted with libtool artifacts. */ > + set_program_name (argv[0]); > + bindtextdomain (PACKAGE, LOCALEBASEDIR); > + textdomain (PACKAGE); > + > + enum { HELP_OPTION = CHAR_MAX + 1 }; > + > + static const char *options = "s:d:"; > + static const struct option long_options[] = { > + {"seed", 1, 0, 's'}, > + {"domain", 1, 0, 'd'}, > + {"help", 0, 0, HELP_OPTION}, > + {0, 0, 0, 0} > + }; Lots of long options have disappeared here, eg. "keys-from-stdin" ...

> + struct drv *sdrv = NULL; > + struct drv *ddrv = NULL; > + int c, nr; > + int option_index; > + int spid, dpid; > + > + sg = guestfs_create (); > + if (sg == NULL) { > + fprintf (stderr, _("guestfs_create: failed to create seed handle\n")); > + exit (EXIT_FAILURE); > + } > + > + dg = guestfs_create (); > + if (dg == NULL) { > + fprintf (stderr, _("guestfs_create: failed to create comparison handle\n")); > + exit (EXIT_FAILURE); > + } > + > + argv[0] = diff_bad_case (program_name); > + > + for (;;) { > + c = getopt_long (argc, argv, options, long_options, &option_index); > + if (c == -1) break; > + > + switch (c) { > + case 0: > + if (STREQ (long_options[option_index].name, "keys-from-stdin")) { > + keys_from_stdin = 1; > + } else if (STREQ (long_options[option_index].name, "echo-keys")) { > + echo_keys = 1; > + } else if (STREQ (long_options[option_index].name, "seed")) { > + if (sdrv) { > + fprintf(stderr, _("Only one seed device")); Missing program_name and \n (but see below). > + exit (EXIT_FAILURE); > + } > + sdrv = calloc (1, sizeof (struct drv)); > + if (!sdrv) { > + perror ("malloc"); > + exit (EXIT_FAILURE); > + } > + sdrv->type = drv_d; > + sdrv->nr_drives = -1; > + sdrv->d.guest = optarg; > + sdrv->next = NULL; > + } else if (STREQ (long_options[option_index].name, "domain")) { > + if (ddrv) { > + fprintf (stderr, _("Only one diff device")); > + exit (EXIT_FAILURE); > + } > + ddrv = calloc (1, sizeof (struct drv)); > + if (!ddrv) { > + perror ("malloc"); > + exit (EXIT_FAILURE); > + } > + ddrv->type = drv_d; > + ddrv->nr_drives = -1; > + ddrv->d.guest = optarg; > + ddrv->next = NULL; This code will never be used. When getopt_long has an equivalent short option, it'll use that instead.

> + } else { > + fprintf (stderr, _("%s: unknown long option: %s (%d)\n"), > + program_name, long_options[option_index].name, option_index); > + exit (EXIT_FAILURE); > + } > + break; > + > + case 'c': > + libvirt_uri = optarg; Missing 'break'.

> + case 's': > + if (sdrv) { > + fprintf(stderr, _("Only one seed device")); > + exit (EXIT_FAILURE); > + } > + sdrv = calloc (1, sizeof (struct drv)); > + if (!sdrv) { > + perror ("malloc"); > + exit (EXIT_FAILURE); > + } > + sdrv->type = drv_d; > + sdrv->nr_drives = -1; > + sdrv->d.guest = optarg; > + sdrv->next = NULL; > + break; > + > + case 'd': > + if (ddrv) { > + fprintf (stderr, _("Only one diff device")); > + exit (EXIT_FAILURE); > + } > + ddrv = calloc (1, sizeof (struct drv)); > + if (!ddrv) { > + perror ("malloc"); > + exit (EXIT_FAILURE); > + } > + ddrv->type = drv_d; > + ddrv->nr_drives = -1; > + ddrv->d.guest = optarg; > + ddrv->next = NULL; > + break; The problem with this syntax is that it forces libvirt to be used. In fact, you can only diff libvirt domains. This seems problematic. Why can't we use '-a' and '-d' options for one of the guests and some other options (eg. '--sa' and '--sd') for the other one?

> + case HELP_OPTION: > + diff_usage (EXIT_SUCCESS); > + > + default: > + diff_usage (EXIT_FAILURE); > + } > + } > + > + if (sdrv == NULL) > + diff_usage (EXIT_FAILURE); > + if (ddrv == NULL) > + diff_usage (EXIT_FAILURE); > + > + struct guestfs_add_domain_argv optargs = { > + .bitmask = 0, > + .libvirturi = libvirt_uri, > + .readonly = 1, > + .allowuuid = 1, > + .readonlydisk = "read", > + }; > + nr = guestfs_add_domain_argv (sg, sdrv->d.guest, &optargs); > + if (nr == -1) > + exit (EXIT_FAILURE); > + sdrv->nr_drives = nr; > + nr = guestfs_add_domain_argv (dg, ddrv->d.guest, &optargs); > + if (nr == -1) > + exit (EXIT_FAILURE); > + ddrv->nr_drives = nr; > + > + if (guestfs_launch (sg) == -1) > + exit (EXIT_FAILURE); > + if (guestfs_launch (dg) == -1) > + exit (EXIT_FAILURE); > + > + diff_inspect_mount (sg); > + diff_inspect_mount (dg); > + > + char stempdir[] = "/tmp/sGuestXXXXXX"; > + char dtempdir[] = "/tmp/dGuestXXXXXX"; > + > + if (mkdtemp (stempdir) == NULL) { > + perror ("mkdtemp"); > + exit (EXIT_FAILURE); > + } > + if (mkdtemp (dtempdir) == NULL) { > + perror ("mkdtemp"); > + exit (EXIT_FAILURE); > + } > + > + if (guestfs_mount_local (sg, stempdir, -1) == -1) > + exit (EXIT_FAILURE); > + if (guestfs_mount_local (dg, dtempdir, -1) == -1) > + exit (EXIT_FAILURE); > + > + spid = fork (); > + if (spid == -1) { > + perror ("fork"); > + exit (EXIT_FAILURE); > + } > + if (spid == 0) { > + if (guestfs_mount_local_run (sg) == -1) > + exit (EXIT_FAILURE); > + _exit (EXIT_SUCCESS); > + } > + > + dpid = fork(); > + if (dpid == -1) { > + perror ("fork"); > + exit (EXIT_FAILURE); > + } > + if (dpid == 0) { > + if (guestfs_mount_local_run (dg) == -1) > + exit (EXIT_FAILURE); > + _exit (EXIT_SUCCESS); > + } > + > + const char *dir = argv[optind]; > + char system_arg[BUFSIZ]; > + sprintf (system_arg, "diff -urN %s%s %s%s", stempdir, dir, > + dtempdir, dir); > + sleep (5); ?

> + if (system (system_arg) == -1) > + exit (EXIT_FAILURE); > + > + guestfs_umount_local (sg, GUESTFS_UMOUNT_LOCAL_RETRY, 1, -1); > + waitpid (spid, NULL, 0); > + guestfs_umount_local (dg, GUESTFS_UMOUNT_LOCAL_RETRY, 1, -1); > + waitpid (dpid, NULL, 0); You must check the return value of every call. Also you should remove the temporary directories.