Re: [libvirt] [PATCH 2/4] logical: Create helper virStorageBackendLogicalParseVolDevice
On 01/26/2016 09:46 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 05:21:04PM -0500, John Ferlan wrote:
> Create a helper routine in order to parse the 'device' string contained
> within the generated 'lvs' output string.
>
> A future patch would then be able to avoid the code more cleanly
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_backend_logical.c | 186 +++++++++++++++++++---------------
> 1 file changed, 104 insertions(+), 82 deletions(-)
>
> diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
> index 76ea00a..bf67faf 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -72,21 +72,115 @@ struct virStorageBackendLogicalPoolVolData {
> };
>
> static int
> -virStorageBackendLogicalMakeVol(char **const groups,
> - void *opaque)
> +virStorageBackendLogicalParseVolDevice(virStorageVolDefPtr vol,
> + char **const groups,
> + int nextents,
> + unsigned long long size,
> + unsigned long long length)
> {
You've called the new helper *ParseVolDevice, but it actually does more than
that. I would rather see it like ParseExtents and also move all the extents
related code into this function (parsing length and size)
OK - my goal was to just parse the devices/extents element so that the
next patch could use the 'nextents' to decide whether to call it... The
'nextents' is just the count... Having more than 1 is seen in a
'striped' and 'mirror' lv. A 'thin' lv has 0... A
'thin-pool' and
'linear' each has 1... Names are found in the 'segtype' field, but
it's
otherwise useless. Perhaps originally the thought when adding a parse of
that field was that the 'stripes' value only was valid for stripes and
mirrors, but it seems it's set depending on the count of devices - so it
seems safe to use it for the extents parsing.
FWIW: The "extents" look like:
/dev/hda2(0) <--- linear
or
/dev/sdc1(10240),/dev/sdd1(0) <--- striped
or
thinpool_lv_test_thin_tdata(0) <--- thin-pool
The "extents" gets further 'regcomp' and 'regexec''d to grab
that "(#)"
which is part of the black magic that I cannot explain ;-)
[...]
> + /* vars[0] is skipped */
> + for (i = 0; i < nextents; i++) {
> + size_t j;
> + int len;
> + char *offset_str = NULL;
> +
> + j = (i * 2) + 1;
> + len = vars[j].rm_eo - vars[j].rm_so;
> + p[vars[j].rm_eo] = '\0';
> +
> + if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path,
> + p + vars[j].rm_so, len) < 0)
> + goto cleanup;
> +
> + len = vars[j + 1].rm_eo - vars[j + 1].rm_so;
> + if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0)
> + goto cleanup;
> +
> + if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed volume extent offset value"));
> + VIR_FREE(offset_str);
> + goto cleanup;
> + }
> +
> + VIR_FREE(offset_str);
> +
> + vol->source.extents[vol->source.nextent].start = offset * size;
> + vol->source.extents[vol->source.nextent].end = (offset * size) +
length;
> + vol->source.nextent++;
This would be much nicer to be done using VIR_APPEND_ELEMENT(). I know that
this commit is just a code movement so this should be done in separate commit.
Yes I pretty much blindly moved "just" the devices parsing code, but I
will note that we're not appending here - it's filling it in. Back in
the caller "vol->source.extents" is allocated using
"vol->source.nextents + nextents". A "new" vol would have
"source.nextents == 0". All this loop does is fill in each
source.extents and bump the source.nextents.
In any case, I will move the allocation of source.extents, fetch of
length and size into the helper (and rename it).
John