[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 cn fujitsu com>
> ---
>  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.

[...]

> +const char *libvirt_uri = NULL;
> +
> +static inline char *
> +diff_bad_case (char const *s)
> +{
> +  return (char *) s;
> +}
> +
> +static void __attribute__((noreturn))
> +diff_usage (int status)
> +{
> +  if (status != EXIT_SUCCESS) {
> +    fprintf (stderr, _("Try `%s --help' for more information.\n"),
> +             program_name);
> +  } else {
> +    fprintf (stdout,
> +           _("%s: Show the differences between seed Guest and the others\n"
> +             "Copyright (C) 2012 Fujitsu Limited.\n"
> +             "Copyright (C) 2012 Red Hat Inc.\n"
> +             "Usage:\n"
> +             "  %s [--options] -s domname -d domname path\n"
> +             "Options:\n"
> +             "  -c|--connect uri     Specify libvirt URI for -s and -d options\n"
> +             "  -s|--seed guest      Add seed disk from libvirt guest\n"
> +             "  -d|--domain guest    Add target disk from libvirt guest\n"
> +             "  --keys-from-stdin    Read passphrases from stdin\n"
> +             "  --echo-keys          Don't turn off echo for passphrases\n"
> +             "  --help               Display brief help\n"),
> +             program_name, program_name);
> +  }
> +  exit (status);
> +}
> +
> +/* Make a LUKS map name from the partition name,
> + * eg "/dev/vda2" => "luksvda2"
> + */
> +static void
> +diff_make_mapname (const char *device, char *mapname, size_t len)
> +{
> +  size_t i = 0;
> +
> +  if (len < 5)
> +    abort ();
> +  strcpy (mapname, "luks");
> +  mapname += 4;
> +  len -= 4;
> +
> +  if (STRPREFIX (device, "/dev/"))
> +    i = 5;
> +
> +  for (; device[i] != '\0' && len >= 1; ++i) {
> +    if (c_isalnum (device[i])) {
> +      *mapname++ = device[i];
> +      len--;
> +    }
> +  }
> +
> +  *mapname = '\0';
> +}
> +
> +static void
> +free_strings (char **strings)
> +{
> +  size_t i;
> +
> +  for (i = 0; strings[i] != NULL; ++i)
> +    free (strings[i]);
> +  free (strings);
> +}
> +
> +static size_t
> +count_strings (char **strings)
> +{
> +  size_t i;
> +
> +  for (i = 0; strings[i] != NULL; ++i)
> +    ;
> +  return i;
> +}
> +
> +/* Simple implementation of decryption: look for any crypto_LUKS
> + * partitions and decrypt them, then rescan for VGs.  This only works
> + * for Fedora whole-disk encryption.  WIP to make this work for other
> + * encryption schemes.
> + */
> +static void
> +diff_inspect_do_decrypt (guestfs_h *g)
> +{
> +  char **partitions = guestfs_list_partitions (g);
> +  if (partitions == NULL)
> +    exit (EXIT_FAILURE);
> +
> +  int need_rescan = 0;
> +  size_t i;
> +  for (i = 0; partitions[i] != NULL; ++i) {
> +    char *type = guestfs_vfs_type (g, partitions[i]);
> +    if (type && STREQ (type, "crypto_LUKS")) {
> +      char mapname[32];
> +      diff_make_mapname (partitions[i], mapname, sizeof mapname);
> +
> +      char *key = read_key (partitions[i]);
> +      /* XXX Should we call guestfs_luks_open_ro if readonly flag
> +       * is set?  This might break 'mount_ro'.
> +       */
> +      if (guestfs_luks_open (g, partitions[i], key, mapname) == -1)
> +        exit (EXIT_FAILURE);
> +
> +      free (key);
> +
> +      need_rescan = 1;
> +    }
> +    free (type);
> +  }
> +
> +  free_strings (partitions);
> +
> +  if (need_rescan) {
> +    if (guestfs_vgscan (g) == -1)
> +      exit (EXIT_FAILURE);
> +    if (guestfs_vg_activate_all (g, 1) == -1)
> +      exit (EXIT_FAILURE);
> +  }
> +}
> +
> +static int
> +compare_keys (const void *p1, const void *p2)
> +{
> +  const char *key1 = * (char * const *) p1;
> +  const char *key2 = * (char * const *) p2;
> +
> +  return strcmp (key1, key2);
> +}
> +
> +static int
> +compare_keys_len (const void *p1, const void *p2)
> +{
> +  const char *key1 = * (char * const *) p1;
> +  const char *key2 = * (char * const *) p2;
> +  int c;
> +
> +  c = strlen (key1) - strlen (key2);
> +  if (c != 0)
> +    return c;
> +
> +  return compare_keys (p1, p2);
> +}
> +
> +static void
> +diff_inspect_mount_root (guestfs_h *g, const char *root)
> +{
> +  char **mountpoints = guestfs_inspect_get_mountpoints (g, root);
> +  if (mountpoints == NULL)
> +    exit (EXIT_FAILURE);
> +
> +  /* Sort by key length, shortest key first, so that we end up
> +   * mounting the filesystems in the correct order.
> +   */
> +  qsort (mountpoints, count_strings (mountpoints) / 2, 2 * sizeof (char *),
> +         compare_keys_len);
> +
> +  size_t i;
> +  size_t mount_errors = 0;
> +  for (i = 0; mountpoints[i] != NULL; i += 2) {
> +    int r;
> +    r = guestfs_mount_ro (g, mountpoints[i+1], mountpoints[i]);
> +    if (r == -1) {
> +      /* If the "/" filesystem could not be mounted, give up, else
> +       * just count the errors and print a warning.
> +       */
> +      if (STREQ (mountpoints[i], "/"))
> +        exit (EXIT_FAILURE);
> +      mount_errors++;
> +    }
> +  }
> +
> +  free_strings (mountpoints);
> +
> +  if (mount_errors)
> +    fprintf (stderr, _("%s: some filesystems could not be mounted (ignored)\n"),
> +             program_name);
> +}
> +
> +/* This function implements the -i option. */
> +static void
> +diff_inspect_mount (guestfs_h *g)
> +{
> +  const char *root = NULL;
> +  diff_inspect_do_decrypt (g);
> +
> +  char **roots = guestfs_inspect_os (g);
> +  if (roots == NULL)
> +    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.

> +  if (guestfs_shutdown (sg) == -1)
> +    exit (EXIT_FAILURE);
> +  if (guestfs_shutdown (dg) == -1)
> +    exit (EXIT_FAILURE);
> +
> +  free_drive (sdrv);
> +  free_drive (ddrv);
> +
> +  guestfs_close (sg);
> +  guestfs_close (dg);
> +
> +  exit (EXIT_SUCCESS);
> +}
> diff --git a/po/POTFILES b/po/POTFILES
> index 60887dc..ad58555 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -1,6 +1,7 @@
>  align/domains.c
>  align/scan.c
>  cat/virt-cat.c
> +cat/virt-diff.c
>  cat/virt-filesystems.c
>  cat/virt-ls.c
>  daemon/9p.c
> -- 
> 1.7.12

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]