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 <jfer...@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list