On 02/01/2016 04:48 AM, Pavel Hrdina wrote:
> 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@xxxxxxxxxx>
>>>> ---
>>>> 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".
>
So do you want the regex back in? Doesn't really matter to me either
way. I am curious though about what construct is "/dev/sda,1" - that
is, how is it used?
>>>> - 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();
> }
> }
>
if 'nextents == 0', then we shouldn't get this far ;-)
>
>>
>> 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.
>
In which case VIR_REALLOC_N of some known sized array and then accessing
each element isn't that much different. Since the more common case is
"1" extent the difference between the two is nothing, especially since
in the long run the APPEND macros call virReallocN. I perhaps would feel
different if I didn't know the value of 'nextents' or this code was
adding 1 extent at a time...
Ironically the two places that use VIR_APPEND_ELEMENT_INPLACE have
callers that will use VIR_REALLOC_N (qemuDomainAttachRNGDevice and in a
much more convoluted path via virDomainChrPreAlloc).
John
>>
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list