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?