[libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output

[Pavel Hrdina]

On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote:

[...]

> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/storage/storage_backend_logical.c | 149 ++++++++++++----------------------
>  tests/virstringtest.c                 |  11 +++
>  2 files changed, 62 insertions(+), 98 deletions(-)
> 
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 7c05b6a..3232c08 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -64,8 +64,6 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
>  }
>  
>  
> -#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped"
> -
>  struct virStorageBackendLogicalPoolVolData {
>      virStoragePoolObjPtr pool;
>      virStorageVolDefPtr vol;
> @@ -75,121 +73,76 @@ static int
>  virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol,
>                                          char **const groups)
>  {
> -    int nextents, ret = -1;
> -    const char *regex_unit = "(\\S+)\\((\\S+)\\)";
> -    char *regex = NULL;
> -    regex_t *reg = NULL;
> -    regmatch_t *vars = NULL;
> -    char *p = NULL;
> -    size_t i;
> -    int err, nvars;
> +    int ret = -1;
> +    char **extents = NULL;
> +    char *extnum, *end;
> +    size_t i, nextents = 0;
>      unsigned long long offset, size, length;
>  
> -    nextents = 1;
> -    if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) {
> -        if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("malformed volume extent stripes value"));
> -            goto cleanup;
> -        }
> -    }
> +    /* The 'devices' (or extents) are split by a comma ",", so let's split
> +     * each out into a parseable string. Since our regex to generate this
> +     * data is "(\\S+)", we can safely assume at least one exists. */
> +    if (!(extents = virStringSplitCount(groups[3], ",", 0, &nextents)))
> +        goto cleanup;

At first I thought of the same solution but what if the device path contains
a comma?  I know, it's very unlikely but it's possible.

>      /* Allocate and fill in extents information */
>      if (VIR_REALLOC_N(vol->source.extents,
>                        vol->source.nextent + nextents) < 0)
>          goto cleanup;
> +    vol->source.nextent += nextents;

I've also mentioned, that this could be done using VIR_APPEND_ELEMENT by
dropping this pre-allocation and ... [1]

>  
> -    if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) {
> +    if (virStrToLong_ull(groups[5], NULL, 10, &length) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         "%s", _("malformed volume extent length value"));
>          goto cleanup;
>      }
>  
> -    if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) {
> +    if (virStrToLong_ull(groups[6], NULL, 10, &size) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         "%s", _("malformed volume extent size value"));
>          goto cleanup;
>      }
>  
> -    if (VIR_STRDUP(regex, regex_unit) < 0)
> -        goto cleanup;
> -
> -    for (i = 1; i < nextents; i++) {
> -        if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2) < 0)
> -            goto cleanup;
> -        /* "," is the separator of "devices" field */
> -        strcat(regex, ",");
> -        strncat(regex, regex_unit, strlen(regex_unit));
> -    }

I would rather allocate the string with correct size outside of the cycle as we
know the length and just simply fill the string in a cycle.  The regex is more
robust than splitting the string by comma.

[...]

> -
> -        vol->source.extents[vol->source.nextent].start = offset * size;
> -        vol->source.extents[vol->source.nextent].end = (offset * size) + length;
> -        vol->source.nextent++;
> +        vol->source.extents[i].start = offset * size;
> +        vol->source.extents[i].end = (offset * size) + length;

[1] ... there you would call VIR_APPEND_ELEMENT().

>      }
> -
>      ret = 0;
>  
>   cleanup:
> -    VIR_FREE(regex);
> -    VIR_FREE(reg);
> -    VIR_FREE(vars);
> +    virStringFreeList(extents);
> +
>      return ret;

The cleanup is nice, but still I would rather use VIR_APPEND_ELEMENT as it's
more common in libvirt to create a new array of some elements.

One more question about the mirror device name, it's not actually a device name
but internal name used by lvm and it's mapped to real device.  Shouldn't we do
the job for user and display a real device?

Pavel