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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list