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

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



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


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