11:01 a.m.
On 06/14/2012 05:14 AM, Sebastian Wiedenroth wrote:
This patch brings support to manage sheepdog pools and volumes to libvirt.
It uses the "collie" command-line utility that comes with sheepdog for that.
Picking up where I left off yesterday...
tests/Makefile.am | 13 ++
tests/storagebackendsheepdogtest.c | 196 ++++++++++++++++
The built executable should be ignored in .gitignore. But thanks for
adding tests!
+if test "$with_storage_sheepdog" = "yes" || test
"$with_storage_sheepdog" = "check"; then
+ AC_PATH_PROG([COLLIE], [collie], [], [$PATH:/sbin:/usr/sbin])
+
+ if test "$with_storage_sheepdog" = "yes" ; then
+ if test -z "$COLLIE" ; then AC_MSG_ERROR([We need collie for Sheepdog
storage driver]) ; fi
I didn't mention this yesterday, but I find it easier to avoid one-liner
'if...fi' and instead use multiple lines - it's easier to see the
nesting if the fi always lines up with the if above it. Besides, that
also avoids going over 80 columns.
+ else
+ if test -z "$COLLIE" ; then with_storage_sheepdog=no ; fi
+
+ if test "$with_storage_sheepdog" = "check" ; then
with_storage_sheepdog=yes ; fi
+ fi
+
+ if test "$with_storage_sheepdog" = "yes" ; then
+ AC_DEFINE_UNQUOTED([WITH_STORAGE_SHEEPDOG], 1, [whether Sheepdog backend for storage
driver is enabled])
Another long line issue; autoconf lets you start arguments on a new line.
+static int
+virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
Now back to where I left off.
+ virStoragePoolObjPtr pool,
+ virStorageVolDefPtr vol,
+ unsigned int flags)
+{
+
+ virCheckFlags(0, -1);
+
+ virCommandPtr cmd = virCommandNew(COLLIE);
+
+ virCommandAddArgList(cmd, "vdi", "delete", vol->name, NULL);
Another instance where these two lines can be merged into one with
virCommandNewArgList. I won't point out any more.
+int
+virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol,
+ char *output)
+{
+ /* example output:
+ * s test 1 10 0 0 1336556634 7c2b25
+ * s test 2 10 0 0 1336557203 7c2b26
+ * = test 3 10 0 0 1336557216 7c2b27
+ */
It might also help to call out another comment describing the fields
represented in that output, as in:
type name id capacity allocation extra1 extra2
+
+ /* skip name */
+ while (*p != '\0'
+ && (*p != ' ' || (*p == ' ' && (*(p - 1)
== '\\'))))
+ ++p;
This won't work if "\\" is a valid escape for a trailing backslash as
the last byte in the name. If you are going to handle backslash escapes
as part of a name, you have to skip all backslash escapes rather than
just searching for the first space not preceded by a backslash.
+ VIR_FREE(vol->target.path);
+ if (virAsprintf(&vol->target.path, "sheepdog:%s", vol->name) ==
-1) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+
+
+cleanup:
Lots of whitespace between lines here.
diff --git a/tests/storagebackendsheepdogtest.c
b/tests/storagebackendsheepdogtest.c
new file mode 100644
index 0000000..a8f62a4
--- /dev/null
+++ b/tests/storagebackendsheepdogtest.c
@@ -0,0 +1,196 @@
+#include <config.h>
Missing a copyright notice. Yeah, we aren't all that great in the tests
directory (and I need to audit the entire source tree for improvements),
but that's no excuse for new additions.
+ output = strdup(test.output);
+ if(!output) {
+ goto cleanup;
+ }
Formatting: space after 'if'. Also, for this one-liner, {} can be omitted.
+
+ ret = virStorageBackendSheepdogParseNodeInfo(pool, output);
+
+ if (ret != test.expected_return)
+ goto cleanup;
+
+ if (ret != 0) {
+ ret = 0;
+ goto cleanup;
+ }
Why are you quitting the test early but still reporting success? Oh, I
see, because you use ret for two orthogonal purposes (checking expected
parse results, and returning test results). I would have written:
if (virStorageBackendSheepdogParseNodeInfo(pool, output) !=
test.expected_return)
goto cleanup;
if (test.expected_return) {
ret = 0;
goto cleanup;
}
so that ret is only being used for test results.
+
+ if (pool->capacity == test.expected_capacity
+ && pool->allocation == test.expected_allocation)
Style - libvirt prefers '&&' on the end of the first line, not the start
of the second line.
+static int
+test_vdi_list_parser(collie_test test, char *poolxml, char *volxml)
+{
+ output = strdup(test.output);
+ if(!output) {
Style: space after 'if'.
+++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml
@@ -0,0 +1,17 @@
+<volume>
+ <name>test2</name>
+ <key>(null)</key>
Is this the sign of an accidental NULL pointer dereference which would
bite us on non-glibc platforms? Can we provide a non-NULL key instead?
Overall, I think my findings are relatively minor, but I would prefer if
you could polish up my findings and post a v3 to save me some time.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org