On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote: [...]
Signed-off-by: John Ferlan <jferlan@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