[libvirt] [PATCH V5] Sheepdog: Adding volume and on pool and refresh.

[Daniel P. Berrange]

On Fri, Feb 07, 2014 at 12:45:42PM +0100, joel SIMOES wrote:

> +static int
> +virStorageBackendSheepdogAddVolume(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                  virStoragePoolObjPtr pool, const char *diskInfo)
> +{
> +    virStorageVolDefPtr vol = NULL;
> +
> +    if (diskInfo == NULL)
> +        goto error;

Missing error report

> +
> +    if (VIR_ALLOC(vol) < 0 || VIR_STRDUP(vol->name, diskInfo) < 0)
> +        goto error;
> +
> +    vol->type = VIR_STORAGE_VOL_NETWORK;
> +
> +    if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0)
> +        goto error;
> +
> +    pool->volumes.objs[pool->volumes.count - 1] = vol;
> +
> +    if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0)
> +        goto error;

If refreshing fails, when we free 'vol', but we've already added
it to the pool->volumes array. So the refresh call needs to be
moved up a few lines.

> +
> +    return 0;
> +
> +error:
> +    virStorageVolDefFree(vol);
> +    return -1;
> +}
> +
> +static int
> +virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                       virStoragePoolObjPtr pool)
> +{
> +    int ret = -1;
> +    char *output = NULL;
> +    char **lines = NULL;
> +    char **cells = NULL;
> +    size_t i;
> +
> +    virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", "-r", NULL);
> +    virStorageBackendSheepdogAddHostArg(cmd, pool);
> +    virCommandSetOutputBuffer(cmd, &output);
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    lines = virStringSplit(output, "\n", 0);
> +    if (lines == NULL)
> +        goto cleanup;
> +
> +    for (i = 0; lines[i]; i++) {
> +        char *line = lines[i];

This could be marked const.

> +        if (line == NULL)
> +            break;
> +
> +        cells = virStringSplit(line, " ", 0);
> +
> +        if (cells != NULL && virStringListLength(cells) > 2) {
> +            if (virStorageBackendSheepdogAddVolume(conn, pool, cells[1]) < 0)
> +                goto cleanup;
> +        }
> +
> +        virStringFreeList(cells);
> +        cells = NULL;
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    virCommandFree(cmd);
> +    virStringFreeList(lines);
> +    virStringFreeList(cells);

We need VIR_FREE(output) here.

> +    return ret;
> +}


I'm going to commit this with the following change applied:


diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
index 08e5473..82df274 100644
--- a/src/storage/storage_backend_sheepdog.c
+++ b/src/storage/storage_backend_sheepdog.c
@@ -115,22 +115,25 @@ virStorageBackendSheepdogAddVolume(virConnectPtr conn ATTRIBUTE_UNUSED,
 {
     virStorageVolDefPtr vol = NULL;
 
-    if (diskInfo == NULL)
+    if (diskInfo == NULL) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Missing disk info when adding volume"));
         goto error;
+    }
 
     if (VIR_ALLOC(vol) < 0 || VIR_STRDUP(vol->name, diskInfo) < 0)
         goto error;
 
     vol->type = VIR_STORAGE_VOL_NETWORK;
 
+    if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0)
+        goto error;
+
     if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0)
         goto error;
 
     pool->volumes.objs[pool->volumes.count - 1] = vol;
 
-    if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0)
-        goto error;
-
     return 0;
 
 error:
@@ -159,7 +162,7 @@ virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED,
         goto cleanup;
 
     for (i = 0; lines[i]; i++) {
-        char *line = lines[i];
+        const char *line = lines[i];
         if (line == NULL)
             break;
 
@@ -180,6 +183,7 @@ cleanup:
     virCommandFree(cmd);
     virStringFreeList(lines);
     virStringFreeList(cells);
+    VIR_FREE(output);
     return ret;
 }
 


Thanks for your contribution !

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|