[libvirt] [PATCH] Sheepdog: Auto Adding volume and correct refresh volume problem.

[Eric Blake]

On 01/22/2014 08:19 AM, joel SIMOES wrote:
> From: Joel SIMOES <joel.simoes at laposte.net>
> 

The commit message body is a great place to give more details about what
the actual "refresh volume problem" is that you encountered

> ---
>  src/storage/storage_backend_sheepdog.c | 91 ++++++++++++++++++++++++++++++----
>  1 file changed, 81 insertions(+), 10 deletions(-)
> 
> diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
> index a6981ce..fbed11a 100644
> --- a/src/storage/storage_backend_sheepdog.c
> +++ b/src/storage/storage_backend_sheepdog.c
> @@ -86,7 +86,8 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool,
>          pool->available = pool->capacity - pool->allocation;
>          return 0;
>  
> -    } while ((p = next));
> +    }
> +    while ((p = next));

This hunk is not needed.


> +static int
> +virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                       virStoragePoolObjPtr pool)
> +{
> +    int ret;
> +    char *output = NULL;
> +
> +    virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", "-r", NULL);
> +    virStorageBackendSheepdogAddHostArg(cmd, pool);
> +    virCommandSetOutputBuffer(cmd, &output);
> +    ret = virCommandRun(cmd, NULL);
> +    char** lines;
> +    char** cells;

Style wise, I'd hoist these two declarations to the top with the other
declarations (in this particular case, your code is syntactically valid
and won't trigger any compiler warnings, but if a later person inserts a
'goto cleanup' prior to these lines, the compiler might start warning
about skipped variable declarations, which is why our HACKING documents
declarations up front in C89 style even though we require a C99 compiler).

> +
> +    if (ret < 0)
> +        goto cleanup;
> +
> +    lines = virStringSplit(output, "\n", 0);
> +    size_t i;

And _this_ declaration is indeed after a conditional goto, also worth
hoisting.

> +    for (i = 0; *(lines + i); i++) {

*(lines + i) is more idiomatically written as lines[i]

> +        char *line = *(lines + i);
> +        cells = virStringSplit(line, " ", 0);
> +        size_t j;
> +        for (j = 0; *(cells + j); j++) {

cells[j]

> +
> +            char *cell = *(cells + j);
> +            if (j == 1) {
> +                virStorageVolDefPtr vol = NULL;
> +                if (VIR_ALLOC(vol) < 0)
> +                    goto cleanup;
> +
> +                if (VIR_STRDUP(vol->name, cell) < 0)
> +                    goto cleanup;

Memory leak.  vol is allocated but goes out of scope by way of the goto.

> +
> +                vol->type = VIR_STORAGE_VOL_BLOCK;
> +
> +                if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0)
> +                    goto cleanup;

More memory leaks.

> +                pool->volumes.objs[pool->volumes.count - 1] = vol;
> +
> +                if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0)
> +                    goto cleanup;
> +
> +                vol = NULL;
> +            }
> +
> +            VIR_FREE(*(cells + j));

Again, VIR_FREE(cells[j]) looks nicer.

...

>  
> +    ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output);
> +    virStorageBackendSheepdogRefreshAllVol(conn, pool);

Why are you ignoring the return status of this call?

> @@ -143,12 +210,14 @@ virStorageBackendSheepdogDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
>      virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "delete", vol->name, NULL);
>      virStorageBackendSheepdogAddHostArg(cmd, pool);
>      int ret = virCommandRun(cmd, NULL);
> +    if (ret < 0)
> +        goto cleanup;
>  
> +cleanup:

This hunk does nothing, and isn't needed.

>      virCommandFree(cmd);
>      return ret;
>  }
>  
> -
>  static int

Spurious whitespace change (while we don't enforce it, most of our code
uses two blank lines between functions).

> @@ -195,12 +263,12 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn,
>          goto cleanup;
>  
>      ret = 0;
> +
>  cleanup:
>      virCommandFree(cmd);
>      return ret;
>  }
>  
> -
>  int

More spurious whitespace changes.

>  virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol,
>                                        char *output)
> @@ -257,7 +325,8 @@ virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol,
>              return -1;
>  
>          return 0;
> -    } while ((p = next));
> +    }
> +    while ((p = next));

Another spurious hunk.

> @@ -310,7 +378,10 @@ virStorageBackendSheepdogResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED,
>      virCommandAddArgFormat(cmd, "%llu", capacity);
>      virStorageBackendSheepdogAddHostArg(cmd, pool);
>      int ret = virCommandRun(cmd, NULL);
> +    if (ret < 0)
> +        goto cleanup;
>  
> +cleanup:

Another no-op change.

-- 
Eric Blake   eblake 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: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140124/0ba4d016/attachment-0001.sig>