3:48 a.m.
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>
>> ---
>> 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