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

Re: [Libguestfs] [PATCH] New API: mdadm-detail



On Wed, Nov 16, 2011 at 05:54:24PM +0000, Matthew Booth wrote:
> 
> Return a hash containing metadata about a specific Linux MD device, based on the
> output of 'mdadm -DY'.
> ---
>  daemon/md.c                    |   78 ++++++++++++++++++++++++++++++++++++++++
>  generator/generator_actions.ml |   31 ++++++++++++++++
>  regressions/test-mdadm.sh      |   60 ++++++++++++++++++++++++++++++-
>  src/MAX_PROC_NR                |    2 +-
>  4 files changed, 169 insertions(+), 2 deletions(-)
> 

> diff --git a/daemon/md.c b/daemon/md.c
> index 257bd0f..c1f9d2a 100644
> --- a/daemon/md.c
> +++ b/daemon/md.c
> @@ -23,6 +23,7 @@
>  #include <string.h>
>  #include <inttypes.h>
>  #include <glob.h>
> +#include <ctype.h>
>  
>  #include "daemon.h"
>  #include "actions.h"
> @@ -230,3 +231,80 @@ error:
>    if (r != NULL) free_strings (r);
>    return NULL;
>  }
> +
> +char **
> +do_mdadm_detail(const char *md)
> +{
> +  int r;
> +  char *out, *err;
> +
> +  char **ret = NULL;
> +  int size = 0, alloc = 0;
> +
> +  const char *mdadm[] = { "mdadm", "-DY", md, NULL };
> +  r = commandv(&out, &err, mdadm);

Any reason not to use plain ol' "command" here?  It takes a
variable number of args.

> +  if (r == -1) {
> +    reply_with_error("%s", err);
> +    free(err);
> +    free(out);
> +    return NULL;
> +  }

Somewhere about here, free(err) needs to be called.

> +  char *p = out;
> +  
> +  /* Parse the output of mdadm -DY:
> +   * MD_LEVEL=raid1
> +   * MD_DEVICES=2
> +   * MD_METADATA=1.0
> +   * MD_UUID=cfa81b59:b6cfbd53:3f02085b:58f4a2e1
> +   * MD_NAME=localhost.localdomain:0
> +   */
> +  while (*p) {
> +    /* Turn the newline (if any) into a null */
> +    char *nl = strchrnul(p, '\n');
> +    if (*nl == '\n') {
> +      *nl = '\0';
> +      nl++;
> +    }

There is a function 'split_lines' defined in daemon.h which
essentially does this (not skipping blank lines though, but
I don't think those are really possible in mdadm output).

> +    /* Skip blank lines (shouldn't happen) */
> +    if (!*p) continue;
> +
> +    /* Split the line in 2 at the equals sign */
> +    char *eq = strchr(p, '=');
> +    if (eq) {
> +      *eq = '\0'; eq++;
> +
> +      /* Remove the MD_ prefix from the key and translate the remainder to lower
> +       * case */
> +      if (STRPREFIX(p, "MD_")) {
> +        p += 3;
> +        for (char *i = p; *i != '\0'; i++) {
> +          *i = tolower(*i);

Probably c_tolower (from "c-ctype.h") is better to use here, since
you're really dealing with 7-bit ASCII in octets, not characters.

> +        }
> +      }
> +
> +      /* Add the key/value pair to the output */
> +      if (add_string(&ret, &size, &alloc, p) == -1) {
> +        free(out);
> +        return NULL;
> +      }
> +      if (add_string(&ret, &size, &alloc, eq) == -1) {
> +        free(out);
> +        return NULL;
> +      }
> +    } else {
> +      /* Ignore lines with no equals sign (shouldn't happen). Log to stderr so
> +       * it will show up in LIBGUESTFS_DEBUG. */
> +      fprintf(stderr, "Unexpected output from mdadm -DY: %s", p);

It's more consistent if the error message is something like:

  "mdadm-delete: unexpected output ignored: %s"

> +    }
> +
> +    p = nl;
> +  }
> +
> +  free(out);
> +
> +  if (add_string(&ret, &size, &alloc, NULL) == -1) return NULL;
> +
> +  return ret;
> +}
> diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
> index a4658a0..9bf9def 100644
> --- a/generator/generator_actions.ml
> +++ b/generator/generator_actions.ml
> @@ -6496,6 +6496,37 @@ If not set, this defaults to C<raid1>.
>     "\
>  List all Linux md devices.");
>  
> +  ("mdadm_detail", (RHashtable "info", [Device "md"], []), 301,  [Optional "mdadm"],
> +   [],
> +   "obtain metadata for an MD device",
> +   "\
> +This command exposes the output of 'mdadm -DY <md>'. The resulting hash will
> +contain at least:

I wouldn't make any specific claims about what mdadm can return.  I
would say something like:

  "The following fields are usually present in the returned hash.
  Other fields may also be present."

> +=over
> +
> +=item C<level>
> +
> +The raid level of the MD device.
> +
> +=item C<devices>
> +
> +The number of underlying devices in the MD device.
> +
> +=item C<metadata>
> +
> +The metadata version used.
> +
> +=item C<uuid>
> +
> +The UUID of the MD device.
> +
> +=item C<name>
> +
> +The name of the MD device.
> +
> +=back");
> +
>  ]
>  
>  let all_functions = non_daemon_functions @ daemon_functions
> diff --git a/regressions/test-mdadm.sh b/regressions/test-mdadm.sh
> index 3ad4f22..8119561 100755
> --- a/regressions/test-mdadm.sh
> +++ b/regressions/test-mdadm.sh
> @@ -92,4 +92,62 @@ write /r5t3/baz "testing"
>  
>  EOF
>  
> -rm -f md-test1.img md-test2.img md-test3.img md-test4.img
> +eval `../fish/guestfish --listen`
> +../fish/guestfish --remote add-ro md-test1.img
> +../fish/guestfish --remote add-ro md-test2.img
> +../fish/guestfish --remote add-ro md-test3.img
> +../fish/guestfish --remote add-ro md-test4.img
> +../fish/guestfish --remote run
> +
> +for md in `../fish/guestfish --remote list-md-devices`; do
> +  ../fish/guestfish --remote mdadm-detail "${md}" > mdadm-detail.out
> +
> +  sed 's/:\s*/=/' mdadm-detail.out > mdadm-detail.out.sh
> +  . mdadm-detail.out.sh
> +  rm -f mdadm-detail.out.sh
> +
> +  error=0
> +  case "$name" in
> +    *:r1t1)
> +      [ "$level" == "raid1" ] || error=1
> +      [ "$devices" == "2" ] || error=1
> +      ;;
> +
> +    *:r1t2)
> +      [ "$level" == "raid1" ] || error=1
> +      [ "$devices" == "2" ] || error=1
> +      ;;
> +
> +    *:r5t1)
> +      [ "$level" == "raid5" ] || error=1
> +      [ "$devices" == "4" ] || error=1
> +      ;;
> +
> +    *:r5t2)
> +      [ "$level" == "raid5" ] || error=1
> +      [ "$devices" == "3" ] || error=1
> +      ;;
> +
> +    *:r5t3)
> +      [ "$level" == "raid5" ] || error=1
> +      [ "$devices" == "2" ] || error=1
> +      ;;
> +
> +    *)
> +      error=1
> +  esac
> +
> +  [[ "$uuid" =~ ([0-9a-f]{8}:){3}[0-9a-f]{8} ]] || error=1
> +  [ ! -z "$metadata" ] || error=1
> +
> +  if [ "$error" == "1" ]; then
> +    echo "$0: Unexpected output from mdadm-detail for device $md"
> +    cat mdadm-detail.out
> +    ../fish/guestfish --remote exit
> +    exit 1
> +  fi
> +done
> +
> +../fish/guestfish --remote exit
> +
> +rm -f mdadm-detail.out md-test1.img md-test2.img md-test3.img md-test4.img

Looks good.  Did the test run OK?

> diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> index 697cb3a..d8fc48a 100644
> --- a/src/MAX_PROC_NR
> +++ b/src/MAX_PROC_NR
> @@ -1 +1 @@
> -300
> +301

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]