[libvirt] [PATCH v2] storage backend: Add sheepdog support

[Eric Blake]

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120620/4127b615/attachment-0001.sig>