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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org