[PATCH] New API: mdadm-detail
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(-)
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
On 17/11/11 09:31, Richard W.M. Jones wrote:
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.
It's slightly more efficient under the hood (no need for malloc) and not especially untidy. I have no strong opinion.
+ if (r == -1) { + reply_with_error("%s", err); + free(err); + free(out); + return NULL; + }
Somewhere about here, free(err) needs to be called.
I was using some of your code from ext2.c as a template here. I hadn't actually bothered to check, but I assumed that as err wasn't freed, it was never assigned. Checking the code, it seems it is always allocated, which means there's the same leak in do_tune2fs_l. I'll update it here.
+ 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).
Looks good. I'll use it. You may want to consider using it in do_tune2fs_l.
+ /* 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.
Ok.
+ } + } + + /* 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"
Ok.
+ } + + 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."
Ok.
+=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?
Yes. The test ran fine. Updated patch to follow. Thanks, Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
Return a hash containing metadata about a specific Linux MD device, based on the output of 'mdadm -DY'. --- daemon/md.c | 80 ++++++++++++++++++++++++++++++++++++++++ generator/generator_actions.ml | 31 +++++++++++++++ regressions/test-mdadm.sh | 60 +++++++++++++++++++++++++++++- src/MAX_PROC_NR | 2 +- 4 files changed, 171 insertions(+), 2 deletions(-)
Thanks, I've applied both this one and the vim patch. Just running the tests before I push them. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
participants (2)
-
Matthew Booth -
Richard W.M. Jones