On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote:
[...]
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list