[Libguestfs] [PATCH] inspection: Handle MD devices in fstab

[Richard W.M. Jones]

On Tue, Nov 22, 2011 at 04:31:50PM +0000, Matthew Booth wrote:
> diff --git a/src/inspect.c b/src/inspect.c
> index 1b41be5..f9b8298 100644
> --- a/src/inspect.c
> +++ b/src/inspect.c
> @@ -61,8 +61,7 @@ guestfs__inspect_os (guestfs_h *g)
>     * information to the handle.
>     */
>    /* Look to see if any devices directly contain filesystems (RHBZ#590167). */
> -  char **devices;
> -  devices = guestfs_list_devices (g);
> +  char **devices = guestfs_list_devices (g);
>    if (devices == NULL)
>      return NULL;
>  
> @@ -77,8 +76,7 @@ guestfs__inspect_os (guestfs_h *g)
>    guestfs___free_string_list (devices);
>  
>    /* Look at all partitions. */
> -  char **partitions;
> -  partitions = guestfs_list_partitions (g);
> +  char **partitions = guestfs_list_partitions (g);
>    if (partitions == NULL) {
>      guestfs___free_inspect_info (g);
>      return NULL;

If there's not any need for these two hunks, then omit them.  OTOH if
it's a good stylistic thing to have them, they should be in a separate
patch so that it can be cherry picked for the stable branch (since all
NFC code cleanups are candidates for cherry picking).

> @@ -141,9 +146,23 @@ 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);
> -static char *resolve_fstab_device (guestfs_h *g, const char *spec);
> -static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const char *filename, int (*f) (guestfs_h *, struct inspect_fs *));
> +                            const char *spec, const char *mp,
> +                            Hash_table *md_map);
> +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 *));

> +static size_t uuid_hash(const void *x, size_t table_size);
> +static bool uuid_cmp(const void *x, const void *y);
> +static void md_uuid_free(void *x);
> +static bool parse_uuid(const char *str, uint32_t *uuid);
> +
> +static size_t mdadm_app_hash(const void *x, size_t table_size);
> +static bool mdadm_app_cmp(const void *x, const void *y);
> +static void mdadm_app_free(void *x);
> +
> +static bool map_app_md_devices (guestfs_h *g, Hash_table **map);
> +static bool map_md_devices(guestfs_h *g, Hash_table **map);

There's a mix of stylistic changes, extensions to core functions (such
as allowing inspect_with_augeas to take a list), and new functionality
(uuid_hash).  To make this easy to backport that could be two or three
separate layers of patches.

> @@ -684,14 +707,15 @@ check_hostname_freebsd (guestfs_h *g, struct inspect_fs *fs)
>  static int
>  check_fstab (guestfs_h *g, struct inspect_fs *fs)
>  {
> +  Hash_table *md_map;
> +  if (!map_md_devices (g, &md_map)) return -1;

This doesn't call error/perrorf() on the error path.

It would be nice if we could enforce this invariant somehow (OCaml can
do this, for example).  I don't know how to in C.

The hash could also do with a small comment to explain briefly what
md_map contains.

> +        case 0:
> +          /* Duplicate uuid in for md device is weird, but not fatal. */
> +          warning(g, "inspect-os: md devices %s and %s have the same uuid",
> +                  ((md_uuid *)matched)->path, entry->path);

I would avoid using warning() -- it says as much in the comment above
the guestfs___warning function in src/guestfs.c.  This one might be a
case for debug() instead.

That's all for now, I'll probably have more to say when I try it out.

Could all the md stuff be moved into another source file?  Might be
cleaner ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora