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@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.
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