[Libguestfs] [PATCH] New API: mdadm-detail
Matthew Booth
mbooth at redhat.com
Thu Nov 17 10:12:09 UTC 2011
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
More information about the Libguestfs
mailing list