[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V3]Sheepdog: Auto Adding volume and pool refresh.



On 01/31/2014 01:02 PM, joel SIMOES wrote:
> From: Joel <joel simoes laposte net>
> 
> Fix bug 1055479
> https://bugzilla.redhat.com/show_bug.cgi?id=1055479
> Libvirt lose sheepdogs volumes on pool refresh or restart.
> When restarting sheepdog pool, all volumes are missing.
> This patch add automatically all volume from the added pool.
> ---
>  src/storage/storage_backend_sheepdog.c | 74 +++++++++++++++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
> index a6981ce..a836f27 100644
> --- a/src/storage/storage_backend_sheepdog.c
> +++ b/src/storage/storage_backend_sheepdog.c
> @@ -109,6 +109,70 @@ virStorageBackendSheepdogAddHostArg(virCommandPtr cmd,
>      virCommandAddArgFormat(cmd, "%d", port);
>  }
>  
> +static int
> +virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                       virStoragePoolObjPtr pool)
> +{
> +    int ret;

int ret = -1;

> +    char *output = NULL;

'lines' and 'cells' should be declared here as well, not below the virCommand
calls.

> +
> +    virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list", "-r", NULL);
> +    virStorageBackendSheepdogAddHostArg(cmd, pool);
> +    virCommandSetOutputBuffer(cmd, &output);

> +    ret = virCommandRun(cmd, NULL);

No need to save the return value, see [1]

> +    char** lines;
> +    char** cells;

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

The return value of virStringSplit needs to be checked for NULL.

> +    size_t i;

This should be declared at the beginning of the function.

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

lines[i] looks nicer than *(lines + i), see [2]

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

You can eliminate this whole loop by checking if cells has at least two
entries and using cells[1] directly.

> +
> +            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;
> +
> +                vol->type = VIR_STORAGE_VOL_BLOCK;
> +
> +                if (VIR_EXPAND_N(pool->volumes.objs, pool->volumes.count, 1) < 0)
> +                    goto cleanup;
> +                pool->volumes.objs[pool->volumes.count - 1] = vol;
> +
> +                if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0)
> +                    goto cleanup;
> +
> +                vol = NULL;
> +            }
> +
> +            VIR_FREE(*(cells + j));

No need to free every string separately,

> +        }
> +
> +        VIR_FREE(cells);

use virStringFreeList(cells) instead.

> +        VIR_FREE(*(lines + i));

Same here.

> +    }
> +
> +
> +    VIR_FREE(lines);
> +    ret = 0;
> +
> +cleanup:
> +    virCommandFree(cmd);
> +    VIR_FREE(lines);
> +    VIR_FREE(cells);
> +    return ret;
> +}
>  
>  static int
>  virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> @@ -122,9 +186,15 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>      virStorageBackendSheepdogAddHostArg(cmd, pool);
>      virCommandSetOutputBuffer(cmd, &output);
>      ret = virCommandRun(cmd, NULL);
> -    if (ret == 0)
> -        ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output);
> +    if (ret < 0)
> +        goto cleanup;
>  
> +    ret = virStorageBackendSheepdogParseNodeInfo(pool->def, output);
> +    if (ret < 0)
> +        goto cleanup;

There's no need to overwrite the 'ret' variable with every call.
if (functionCall() < 0)
  goto cleanup;

works too, since most of the functions only return 0 and -1.

> +    ret = virStorageBackendSheepdogRefreshAllVol(conn, pool);
> +
> +cleanup:
>      virCommandFree(cmd);
>      VIR_FREE(output);
>      return ret;
> 

Jan

[1] https://www.redhat.com/archives/libvir-list/2014-January/msg01232.html
[2] https://www.redhat.com/archives/libvir-list/2014-January/msg01215.html

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]