Libguestfs - Nov 2011 - [PATCH] inspection: Handle MD devices in fstab

This patch fixes inspection when fstab contains devices md devices
specified as /dev/mdN. The appliance creates these devices without reference to
the guest's mdadm.conf so, for e.g. /dev/md0 in the guest will often be
created
as /dev/md127 in the appliance. With this patch, we match the uuids of detected
md devices against uuids specified in mdadm.conf, and map them appropriately
when we encounter them in fstab.

The test in this patch also checks that devices specified by their md name, e.g.
/dev/md/boot, work correctly, although the patch isn't required for devices
specified this way.
---
 regressions/Makefile.am              |    1 +
 regressions/test-inspect-fstab-md.sh |   65 ++++++
 src/inspect.c                        |   22 ++-
 src/inspect_fs_unix.c                |  411 ++++++++++++++++++++++++++++++----
 4 files changed, 456 insertions(+), 43 deletions(-)
 create mode 100755 regressions/test-inspect-fstab-md.sh

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-inspection-Handle-MD-devices-in-fstab.patch
Type: text/x-patch
Size: 21031 bytes
Desc: not available
URL:
<http://listman.redhat.com/archives/libguestfs/attachments/20111122/330e6c04/attachment.bin>

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

On 11/23/2011 12:31 AM, Matthew Booth wrote:
> 
> This patch fixes inspection when fstab contains devices md devices
> specified as /dev/mdN. The appliance creates these devices without
reference to
> the guest's mdadm.conf so, for e.g. /dev/md0 in the guest will often be
created
> as /dev/md127 in the appliance. With this patch, we match the uuids of
detected
> md devices against uuids specified in mdadm.conf, and map them
appropriately
> when we encounter them in fstab.
> 
> The test in this patch also checks that devices specified by their md name,
e.g.
> /dev/md/boot, work correctly, although the patch isn't required for
devices
> specified this way.

Sorry to say that I can't apply this big patch to do a test.

Thanks
-Wanlong Gao
> ---
>  regressions/Makefile.am              |    1 +
>  regressions/test-inspect-fstab-md.sh |   65 ++++++
>  src/inspect.c                        |   22 ++-
>  src/inspect_fs_unix.c                |  411
++++++++++++++++++++++++++++++----
>  4 files changed, 456 insertions(+), 43 deletions(-)
>  create mode 100755 regressions/test-inspect-fstab-md.sh
> 
> 
> 
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs