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

Re: [Libguestfs] [PATCH 5/7] inspect: Update inspect_os to use mountables



On Tue, Feb 12, 2013 at 11:04:24AM +0000, Matthew Booth wrote:
> This fixes inspection of guests which use btrfs subvolumes.
> ---
>  src/guestfs-internal.h |   7 +--
>  src/inspect-fs-cd.c    |   2 +-
>  src/inspect-fs-unix.c  | 144 +++++++++++++++++++++++++++----------------------
>  src/inspect-fs.c       |  53 +++++++++---------
>  src/inspect.c          |  12 ++---
>  5 files changed, 118 insertions(+), 100 deletions(-)

Difficult to review this patch.  There seem to be at least
two patches in this:

(1) A patch that just changes device -> mountable.  Easy
to review.

(2) A patch that does the other changes, sans unrelated variable-
movement.

Rich.

> diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
> index bd7c802..c52b9a5 100644
> --- a/src/guestfs-internal.h
> +++ b/src/guestfs-internal.h
> @@ -391,7 +391,7 @@ enum inspect_os_package_management {
>  
>  struct inspect_fs {
>    int is_root;
> -  char *device;
> +  char *mountable;
>    enum inspect_os_type type;
>    enum inspect_os_distro distro;
>    enum inspect_os_package_format package_format;
> @@ -414,7 +414,7 @@ struct inspect_fs {
>  };
>  
>  struct inspect_fstab_entry {
> -  char *device;
> +  char *mountable;
>    char *mountpoint;
>  };
>  
> @@ -524,7 +524,8 @@ extern struct inspect_fs *guestfs___search_for_root (guestfs_h *g, const char *r
>  /* inspect-fs.c */
>  extern int guestfs___is_file_nocase (guestfs_h *g, const char *);
>  extern int guestfs___is_dir_nocase (guestfs_h *g, const char *);
> -extern int guestfs___check_for_filesystem_on (guestfs_h *g, const char *device);
> +extern int guestfs___check_for_filesystem_on (guestfs_h *g,
> +                                              const char *mountable);
>  extern int guestfs___parse_unsigned_int (guestfs_h *g, const char *str);
>  extern int guestfs___parse_unsigned_int_ignore_trailing (guestfs_h *g, const char *str);
>  extern int guestfs___parse_major_minor (guestfs_h *g, struct inspect_fs *fs);
> diff --git a/src/inspect-fs-cd.c b/src/inspect-fs-cd.c
> index e4f257f..407e4f8 100644
> --- a/src/inspect-fs-cd.c
> +++ b/src/inspect-fs-cd.c
> @@ -503,7 +503,7 @@ guestfs___check_installer_iso (guestfs_h *g, struct inspect_fs *fs,
>      return 0;
>  
>    /* Otherwise we matched an ISO, so fill in the fs fields. */
> -  fs->device = safe_strdup (g, device);
> +  fs->mountable = safe_strdup (g, device);
>    fs->is_root = 1;
>    fs->format = OS_FORMAT_INSTALLER;
>    fs->type = osinfo->type;
> diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
> index 40f797d..26c7579 100644
> --- a/src/inspect-fs-unix.c
> +++ b/src/inspect-fs-unix.c
> @@ -160,8 +160,7 @@ static int check_hostname_redhat (guestfs_h *g, struct inspect_fs *fs);
>  static int check_hostname_freebsd (guestfs_h *g, struct inspect_fs *fs);
>  static int check_fstab (guestfs_h *g, struct inspect_fs *fs);
>  static int add_fstab_entry (guestfs_h *g, struct inspect_fs *fs,
> -                            const char *spec, const char *mp,
> -                            Hash_table *md_map);
> +                            const char *mountable, const char *mp);
>  static char *resolve_fstab_device (guestfs_h *g, const char *spec,
>                                     Hash_table *md_map);
>  static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *));
> @@ -870,17 +869,13 @@ check_hostname_freebsd (guestfs_h *g, struct inspect_fs *fs)
>  static int
>  check_fstab (guestfs_h *g, struct inspect_fs *fs)
>  {
> -  CLEANUP_FREE_STRING_LIST char **entries = NULL;
> -  char **entry;
> -  char augpath[256];
> -  int r;
> -  CLEANUP_HASH_FREE Hash_table *md_map;
> -
>    /* Generate a map of MD device paths listed in /etc/mdadm.conf to MD device
>     * paths in the guestfs appliance */
> +  CLEANUP_HASH_FREE Hash_table *md_map = NULL;
>    if (map_md_devices (g, &md_map) == -1) return -1;
>  
> -  entries = guestfs_aug_match (g, "/files/etc/fstab/*[label() != '#comment']");
> +  CLEANUP_FREE_STRING_LIST char **entries =
> +    guestfs_aug_match (g, "/files/etc/fstab/*[label() != '#comment']");
>    if (entries == NULL)
>      return -1;
>  
> @@ -889,20 +884,81 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
>      return -1;
>    }
>  
> -  for (entry = entries; *entry != NULL; entry++) {
> -    snprintf (augpath, sizeof augpath, "%s/spec", *entry);
> -    CLEANUP_FREE char *spec = guestfs_aug_get (g, augpath);
> -    if (spec == NULL)
> -      return -1;
> +  for (char **entry = entries; *entry != NULL; entry++) {
> +    char augpath[256];
>  
>      snprintf (augpath, sizeof augpath, "%s/file", *entry);
>      CLEANUP_FREE char *mp = guestfs_aug_get (g, augpath);
> -    if (mp == NULL)
> -      return -1;
> +    if (mp == NULL) return -1;
> +
> +    /* Ignore certain mountpoints. */
> +    if (STRPREFIX (mp, "/dev/") ||
> +        STREQ (mp, "/dev") ||
> +        STRPREFIX (mp, "/media/") ||
> +        STRPREFIX (mp, "/proc/") ||
> +        STREQ (mp, "/proc") ||
> +        STRPREFIX (mp, "/selinux/") ||
> +        STREQ (mp, "/selinux") ||
> +        STRPREFIX (mp, "/sys/") ||
> +        STREQ (mp, "/sys"))
> +      continue;
>  
> -    r = add_fstab_entry (g, fs, spec, mp, md_map);
> -    if (r == -1)
> -      return -1;
> +    snprintf (augpath, sizeof augpath, "%s/spec", *entry);
> +    CLEANUP_FREE char *spec = guestfs_aug_get (g, augpath);
> +    if (spec == NULL) return -1;
> +
> +    /* Ignore /dev/fd (floppy disks) (RHBZ#642929) and CD-ROM drives. */
> +    if ((STRPREFIX (spec, "/dev/fd") && c_isdigit (spec[7])) ||
> +        STREQ (spec, "/dev/floppy") ||
> +        STREQ (spec, "/dev/cdrom"))
> +      continue;
> +
> +    /* Resolve UUID= and LABEL= to the actual device. */
> +    CLEANUP_FREE char *mountable = NULL;
> +    if (STRPREFIX (spec, "UUID="))
> +      mountable = guestfs_findfs_uuid (g, &spec[5]);
> +    else if (STRPREFIX (spec, "LABEL="))
> +      mountable = guestfs_findfs_label (g, &spec[6]);
> +    /* Ignore "/.swap" (Pardus) and pseudo-devices like "tmpfs". */
> +    else if (STREQ (spec, "/dev/root"))
> +      /* Resolve /dev/root to the current device. */
> +      mountable = safe_strdup (g, fs->mountable);
> +    else if (STRPREFIX (spec, "/dev/"))
> +      /* Resolve guest block device names. */
> +      mountable = resolve_fstab_device (g, spec, md_map);
> +
> +    /* If we haven't resolved the device successfully by this point,
> +     * we don't care, just ignore it.
> +     */
> +    if (mountable == NULL)
> +      continue;
> +
> +    snprintf (augpath, sizeof augpath, "%s/vfstype", *entry);
> +    CLEANUP_FREE char *vfstype = guestfs_aug_get (g, augpath);
> +    if (vfstype == NULL) return -1;
> +
> +    if (STREQ (vfstype, "btrfs")) {
> +      snprintf (augpath, sizeof augpath, "%s/opt", *entry);
> +      CLEANUP_FREE_STRING_LIST char **opts = guestfs_aug_match (g, augpath);
> +      if (opts == NULL) return -1;
> +
> +      for (char **opt = opts; *opt; opt++) {
> +        CLEANUP_FREE char *optname = guestfs_aug_get (g, augpath);
> +        if (optname == NULL) return -1;
> +
> +        if (STREQ (optname, "subvol")) {
> +          snprintf (augpath, sizeof augpath, "%s/value", *opt);
> +          CLEANUP_FREE char *subvol = guestfs_aug_get (g, augpath);
> +          if (subvol == NULL) return -1;
> +
> +          char *new = safe_asprintf (g, "btrfsvol:%s/%s", mountable, subvol);
> +          free (mountable);
> +          mountable = new;
> +        }
> +      }
> +    }
> +
> +    if  (add_fstab_entry (g, fs, mountable, mp) == -1) return -1;
>    }
>  
>    return 0;
> @@ -918,48 +974,8 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
>   */
>  static int
>  add_fstab_entry (guestfs_h *g, struct inspect_fs *fs,
> -                 const char *spec, const char *mp, Hash_table *md_map)
> +                 const char *mountable, const char *mountpoint)
>  {
> -  /* Ignore certain mountpoints. */
> -  if (STRPREFIX (mp, "/dev/") ||
> -      STREQ (mp, "/dev") ||
> -      STRPREFIX (mp, "/media/") ||
> -      STRPREFIX (mp, "/proc/") ||
> -      STREQ (mp, "/proc") ||
> -      STRPREFIX (mp, "/selinux/") ||
> -      STREQ (mp, "/selinux") ||
> -      STRPREFIX (mp, "/sys/") ||
> -      STREQ (mp, "/sys"))
> -    return 0;
> -
> -  /* Ignore /dev/fd (floppy disks) (RHBZ#642929) and CD-ROM drives. */
> -  if ((STRPREFIX (spec, "/dev/fd") && c_isdigit (spec[7])) ||
> -      STREQ (spec, "/dev/floppy") ||
> -      STREQ (spec, "/dev/cdrom"))
> -    return 0;
> -
> -  /* Resolve UUID= and LABEL= to the actual device. */
> -  char *device = NULL;
> -  if (STRPREFIX (spec, "UUID="))
> -    device = guestfs_findfs_uuid (g, &spec[5]);
> -  else if (STRPREFIX (spec, "LABEL="))
> -    device = guestfs_findfs_label (g, &spec[6]);
> -  /* Ignore "/.swap" (Pardus) and pseudo-devices like "tmpfs". */
> -  else if (STREQ (spec, "/dev/root"))
> -    /* Resolve /dev/root to the current device. */
> -    device = safe_strdup (g, fs->device);
> -  else if (STRPREFIX (spec, "/dev/"))
> -    /* Resolve guest block device names. */
> -    device = resolve_fstab_device (g, spec, md_map);
> -
> -  /* If we haven't resolved the device successfully by this point,
> -   * we don't care, just ignore it.
> -   */
> -  if (device == NULL)
> -    return 0;
> -
> -  char *mountpoint = safe_strdup (g, mp);
> -
>    /* Add this to the fstab entry in 'fs'.
>     * Note these are further filtered by guestfs_inspect_get_mountpoints
>     * and guestfs_inspect_get_filesystems.
> @@ -970,8 +986,6 @@ add_fstab_entry (guestfs_h *g, struct inspect_fs *fs,
>    p = realloc (fs->fstab, n * sizeof (struct inspect_fstab_entry));
>    if (p == NULL) {
>      perrorf (g, "realloc");
> -    free (device);
> -    free (mountpoint);
>      return -1;
>    }
>  
> @@ -979,10 +993,10 @@ add_fstab_entry (guestfs_h *g, struct inspect_fs *fs,
>    fs->nr_fstab = n;
>  
>    /* These are owned by the handle and freed by guestfs___free_inspect_info. */
> -  fs->fstab[n-1].device = device;
> -  fs->fstab[n-1].mountpoint = mountpoint;
> +  fs->fstab[n-1].mountable = safe_strdup (g, mountable);
> +  fs->fstab[n-1].mountpoint = safe_strdup (g, mountpoint);
>  
> -  debug (g, "fstab: device=%s mountpoint=%s", device, mountpoint);
> +  debug (g, "fstab: mountable=%s mountpoint=%s", mountable, mountpoint);
>  
>    return 0;
>  }
> diff --git a/src/inspect-fs.c b/src/inspect-fs.c
> index be22eb0..8a88822 100644
> --- a/src/inspect-fs.c
> +++ b/src/inspect-fs.c
> @@ -79,7 +79,8 @@ free_regexps (void)
>    pcre_free (re_major_minor);
>  }
>  
> -static int check_filesystem (guestfs_h *g, const char *device,
> +static int check_filesystem (guestfs_h *g, const char *mountable,
> +                             const struct guestfs_mountable_internal *m,
>                               int whole_device);
>  static int extend_fses (guestfs_h *g);
>  
> @@ -87,7 +88,7 @@ static int extend_fses (guestfs_h *g);
>   * another entry in g->fses.
>   */
>  int
> -guestfs___check_for_filesystem_on (guestfs_h *g, const char *device)
> +guestfs___check_for_filesystem_on (guestfs_h *g, const char *mountable)
>  {
>    CLEANUP_FREE char *vfs_type = NULL;
>    int is_swap, r;
> @@ -98,25 +99,31 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char *device)
>     * temporarily replace the error handler with a null one.
>     */
>    guestfs_push_error_handler (g, NULL, NULL);
> -  vfs_type = guestfs_vfs_type (g, device);
> +  vfs_type = guestfs_vfs_type (g, mountable);
>    guestfs_pop_error_handler (g);
>  
>    is_swap = vfs_type && STREQ (vfs_type, "swap");
>    debug (g, "check_for_filesystem_on: %s (%s)",
> -         device, vfs_type ? vfs_type : "failed to get vfs type");
> +         mountable, vfs_type ? vfs_type : "failed to get vfs type");
>  
>    if (is_swap) {
>      if (extend_fses (g) == -1)
>        return -1;
>      fs = &g->fses[g->nr_fses-1];
> -    fs->device = safe_strdup (g, device);
> +    fs->mountable = safe_strdup (g, mountable);
>      return 0;
>    }
>  
> +  struct guestfs_mountable_internal *m =
> +    guestfs_internal_parse_mountable (g, mountable);
> +
>    /* If it's a whole device, see if it is an install ISO. */
> -  int whole_device = guestfs_is_whole_device (g, device);
> -  if (whole_device == -1) {
> -    return -1;
> +  int whole_device = 0;
> +  if (m->type == MOUNTABLE_DEVICE) {
> +    whole_device = guestfs_is_whole_device (g, m->device);
> +    if (whole_device == -1) {
> +      return -1;
> +    }
>    }
>  
>    if (whole_device) {
> @@ -124,7 +131,7 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char *device)
>        return -1;
>      fs = &g->fses[g->nr_fses-1];
>  
> -    r = guestfs___check_installer_iso (g, fs, device);
> +    r = guestfs___check_installer_iso (g, fs, m->device);
>      if (r == -1) {              /* Fatal error. */
>        g->nr_fses--;
>        return -1;
> @@ -140,19 +147,19 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char *device)
>    guestfs_push_error_handler (g, NULL, NULL);
>    if (vfs_type && STREQ (vfs_type, "ufs")) { /* Hack for the *BSDs. */
>      /* FreeBSD fs is a variant of ufs called ufs2 ... */
> -    r = guestfs_mount_vfs (g, "ro,ufstype=ufs2", "ufs", device, "/");
> +    r = guestfs_mount_vfs (g, "ro,ufstype=ufs2", "ufs", mountable, "/");
>      if (r == -1)
>        /* while NetBSD and OpenBSD use another variant labeled 44bsd */
> -      r = guestfs_mount_vfs (g, "ro,ufstype=44bsd", "ufs", device, "/");
> +      r = guestfs_mount_vfs (g, "ro,ufstype=44bsd", "ufs", mountable, "/");
>    } else {
> -    r = guestfs_mount_ro (g, device, "/");
> +    r = guestfs_mount_ro (g, mountable, "/");
>    }
>    guestfs_pop_error_handler (g);
>    if (r == -1)
>      return 0;
>  
>    /* Do the rest of the checks. */
> -  r = check_filesystem (g, device, whole_device);
> +  r = check_filesystem (g, mountable, m, whole_device);
>  
>    /* Unmount the filesystem. */
>    if (guestfs_umount_all (g) == -1)
> @@ -161,28 +168,24 @@ guestfs___check_for_filesystem_on (guestfs_h *g, const char *device)
>    return r;
>  }
>  
> -/* is_block and is_partnum are just hints: is_block is true if the
> - * filesystem is a whole block device (eg. /dev/sda).  is_partnum
> - * is > 0 if the filesystem is a direct partition, and in this case
> - * it is the partition number counting from 1
> - * (eg. /dev/sda1 => is_partnum == 1).
> - */
>  static int
> -check_filesystem (guestfs_h *g, const char *device, int whole_device)
> +check_filesystem (guestfs_h *g, const char *mountable,
> +                  const struct guestfs_mountable_internal *m,
> +                  int whole_device)
>  {
>    if (extend_fses (g) == -1)
>      return -1;
>  
>    int partnum = -1;
> -  if (!whole_device) {
> +  if (!whole_device && m->type == MOUNTABLE_DEVICE) {
>      guestfs_push_error_handler (g, NULL, NULL);
> -    partnum = guestfs_part_to_partnum (g, device);
> +    partnum = guestfs_part_to_partnum (g, m->device);
>      guestfs_pop_error_handler (g);
>    }
>  
>    struct inspect_fs *fs = &g->fses[g->nr_fses-1];
>  
> -  fs->device = safe_strdup (g, device);
> +  fs->mountable = safe_strdup (g, mountable);
>  
>    /* Optimize some of the tests by avoiding multiple tests of the same thing. */
>    int is_dir_etc = guestfs_is_dir (g, "/etc") > 0;
> @@ -203,7 +206,7 @@ check_filesystem (guestfs_h *g, const char *device, int whole_device)
>       * that is probably /dev/sda5 (see:
>       * http://www.freebsd.org/doc/handbook/disk-organization.html)
>       */
> -    if (match (g, device, re_first_partition))
> +    if (m->type == MOUNTABLE_DEVICE && match (g, m->device, re_first_partition))
>        return 0;
>  
>      fs->is_root = 1;
> @@ -219,7 +222,7 @@ check_filesystem (guestfs_h *g, const char *device, int whole_device)
>       * that is probably /dev/sda5 (see:
>       * http://www.freebsd.org/doc/handbook/disk-organization.html)
>       */
> -    if (match (g, device, re_first_partition))
> +    if (m->type == MOUNTABLE_DEVICE && match (g, m->device, re_first_partition))
>        return 0;
>  
>      fs->is_root = 1;
> diff --git a/src/inspect.c b/src/inspect.c
> index ae746cb..f031434 100644
> --- a/src/inspect.c
> +++ b/src/inspect.c
> @@ -107,7 +107,7 @@ guestfs__inspect_get_roots (guestfs_h *g)
>    count = 0;
>    for (i = 0; i < g->nr_fses; ++i) {
>      if (g->fses[i].is_root) {
> -      ret[count] = safe_strdup (g, g->fses[i].device);
> +      ret[count] = safe_strdup (g, g->fses[i].mountable);
>        count++;
>      }
>    }
> @@ -347,7 +347,7 @@ guestfs__inspect_get_mountpoints (guestfs_h *g, const char *root)
>    for (i = 0; i < nr; ++i)
>      if (CRITERION (fs, i)) {
>        ret[2*count] = safe_strdup (g, fs->fstab[i].mountpoint);
> -      ret[2*count+1] = safe_strdup (g, fs->fstab[i].device);
> +      ret[2*count+1] = safe_strdup (g, fs->fstab[i].mountable);
>        count++;
>      }
>  #undef CRITERION
> @@ -379,7 +379,7 @@ guestfs__inspect_get_filesystems (guestfs_h *g, const char *root)
>    }
>  
>    for (i = 0; i < nr; ++i)
> -    ret[i] = safe_strdup (g, fs->fstab[i].device);
> +    ret[i] = safe_strdup (g, fs->fstab[i].mountable);
>  
>    return ret;
>  }
> @@ -485,7 +485,7 @@ guestfs___free_inspect_info (guestfs_h *g)
>  {
>    size_t i;
>    for (i = 0; i < g->nr_fses; ++i) {
> -    free (g->fses[i].device);
> +    free (g->fses[i].mountable);
>      free (g->fses[i].product_name);
>      free (g->fses[i].product_variant);
>      free (g->fses[i].arch);
> @@ -494,7 +494,7 @@ guestfs___free_inspect_info (guestfs_h *g)
>      free (g->fses[i].windows_current_control_set);
>      size_t j;
>      for (j = 0; j < g->fses[i].nr_fstab; ++j) {
> -      free (g->fses[i].fstab[j].device);
> +      free (g->fses[i].fstab[j].mountable);
>        free (g->fses[i].fstab[j].mountpoint);
>      }
>      free (g->fses[i].fstab);
> @@ -608,7 +608,7 @@ guestfs___search_for_root (guestfs_h *g, const char *root)
>    struct inspect_fs *fs;
>    for (i = 0; i < g->nr_fses; ++i) {
>      fs = &g->fses[i];
> -    if (fs->is_root && STREQ (root, fs->device))
> +    if (fs->is_root && STREQ (root, fs->mountable))
>        return fs;
>    }
>  
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs redhat com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/


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