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

On Fri, Jan 29, 2016 at 12:50:06PM -0500, John Ferlan wrote: > > > On 01/29/2016 11:07 AM, Pavel Hrdina wrote: >> On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote: >> >> [...] >> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> 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. >> > > Well it would have failed with the current code... The existing > algorithm would concatenate 'nsegments' of 'regex_units' separated by a > comma [1]. That's not true, regex is greedy and it tries to match as much as possible: https://regex101.com/r/lS9tZ5/1 Like I said, the regex is more robust and parse the string correctly event with comma as part of a device name, for example "/dev/sda,1".

>>> - 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)); > > [1] > > So for 2 'nextents' it's: > > "(\\S+)\\((\\S+)\\),(\\S+)\\((\\S+)\\)" > > >>> - } >> >> 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. >> > > Not sure I follow the comment here, but I'll point out that part of the > problem with the existing algorithm is that it "assumes" extents=1 and > only allows adjustment if the segtype=="striped". This is bad for > "mirror" now and eventually bad for "thin" later. Well, I meant something like this instead of the current code: if (nextents) { if (VIR_ALLOC_N(regex, ....) goto cleanup; strcpy(); for (....) { strcat(); strncat(); } }

> > I find usage of regex in here an over complication. There's more agita, > allocation, possible errors, etc. spent trying to just set up the regex. > It's one of those - close my eyes and hope it works... > >> [...] >> >>> - >>> - 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(). >> > > I don't find the whole VIR_APPEND_ELEMENT option as clean... But I'll > go with it ... > > Usage will require a change to src/conf/storage_conf.h in order to > change 'nextent' to a 'size_t' (from an 'int') - compiler complains > otherwise. > > Then remove the VIR_REALLOC_N > > Add a memset(&extent, 0, sizeof(extent)); prior to the for loop and one > at the end of the for loop after the successful VIR_APPEND_ELEMENT You don't need to add one after the successful VIR_APPEND_ELEMENT, it does that for us. There is a different version called VIR_APPEND_ELEMENT_COPY to not clean the original element. In this case, where we know the number of elements, we can pre-allocate the array and use VIR_APPEND_ELEMENT_INSERT which requires the array to be allocated.

> > Add the following in cleanup due to : > > if (extent.path) > VIR_FREE(extent.path); > > If you'd like to see the adjusted patch - I can post it. Yes I would like to see the adjusted patch. > >>> } >>> - >>> 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? >> > > Another patch for a different day perhaps... Yes I know and I'm not asking to fix it together with this patch series. I just wanted to note it so there is at least some discussion about it. > > Mirror, raid, and thin lv to "real" device is managed by lvm. I would > think someone knowing how to use the lvs tools would be able to figure > that out... > > It's possible to get using an 'lvs -a -o name,devices $vgname' call and > iterating through, but I'm not sure it'd be worth doing. Yes, it's probably not worth doing it unless someone asks for it. Pavel