[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 08/21/2012 02:44 PM, Richard W.M. Jones wrote:
> 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.

I see.


>> +    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 ...").

OK, thank you.

> 
>> +  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.

OK.

> 
>> +  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" ...

oops.

> 
>> +  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.

Got it.

> 
>> +      } 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'.

thank you.

> 
>> +    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?

Thank you for this suggestion, I'll try it then.

> 
>> +    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);
> 
> ?

I think I'm waiting for guest launching?

> 
>> +  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.

OK, got it. thank you.

Wanlong Gao


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