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

2016-02-01 Thread Pavel Hrdina 
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 
> >> ---
> >>  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, ) < 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, )))
> >> +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
>

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

2016-02-01 Thread Pavel Hrdina 
On Mon, Feb 01, 2016 at 07:27:54AM -0500, John Ferlan wrote:
> > 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?

Yes, I would rather use the regex as it's more robust.  This was just to
demonstrate the comma in device name.  I'm not sure, whether it's possible to
have some storage device with comma in its name, but why to remove code, that's
prepared to handle also this one corner case?

>  -
>  -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(, 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).

Well, the benefit of VIR_APPEND_ELEMENT macros is that it does some error
checking and also memset the original element to 0.  Like I wrote, it's not
different but the code is cleaner and easier to read and since we have those
macros, why don't use them?

I personally like this approach:

virStorageVolSourceExtent extent = { NULL, 0, 0 }; // or use memset to 0

if (VIR_REALLOC_N(vol->source.extents, vol->source.nextent + nextents) < 0)
goto cleanup;

for (i = 0; i < nextents; i++) {
...
if (VIR_STRNDUP(extent.path, ...) < 0)
goto cleanup;
extent.start = ...;
extent.end = ...;
if (VIR_APPEND_ELEMENT_INPLACE(vol->source.extents, vol->source.nextent,
   extent) < 0)
goto cleanup;
}

then the cuurent "ugly" approach  vol->source.extents[vol->source.nextent] .

Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-02-01 Thread John Ferlan 


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 
 ---
  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, ) < 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, )))
 +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 *

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

2016-02-01 Thread John Ferlan 


On 02/01/2016 08:11 AM, Pavel Hrdina wrote:
> On Mon, Feb 01, 2016 at 07:27:54AM -0500, John Ferlan wrote:
>>> 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?
> 
> Yes, I would rather use the regex as it's more robust.  This was just to
> demonstrate the comma in device name.  I'm not sure, whether it's possible to
> have some storage device with comma in its name, but why to remove code, 
> that's
> prepared to handle also this one corner case?
> 

So by removing the virStringSplitCount do you have any suggestions to
get the 'nextents' value?

This patch also removed the 'stripes' value which is supposed to be only
valid on striped and mirror lv's, although it has been found valid for
'linear', 'thin-pool', and 'thin'.

I'm back at square 1 with one adjustment:

nextents = 1;
if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED) ||
STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_MIRROR)) {
if (virStrToLong_i(groups[5], NULL, 10, ) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
   _("malformed volume extent stripes value"));
goto cleanup;
}
}

where patch 6 adds in the following a the top to avoid the setting of
any extents data or search through the now possibly empty devices:

/* If the groups field is NULL, then there's nothing to do */
if (!groups[3])
return 0;


>> -
>> -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(, 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).
> 
> Well, the benefit of VIR_APPEND_ELEMENT macros is that it does some error
> checking and also memset the original element to 0.  Like I wrote, it's not
> different but the code is cleaner and easier to read and since we have those
> macros, why don't use them?
> 

I find the REALLOC_N more readable, you find APPEND more readable. Like
I noted already it's 50/50 in the code REALLOC vs. APPEND. I'm fine with
using the APPEND option.

> I personally like this approach:
> 
> virStorageVolSourceExtent extent = { NULL, 0, 0 }; // or use memset to 0
> 
> if (VIR_REALLOC_N(vol->source.extents, vol->source.nextent + nextents) < 0)
> goto cleanup;
> 
> for (i = 0; i < nextents; i++) {
> ...
> if (VIR_STRNDUP(extent.path, ...) < 0)
> goto cleanup;
> extent.start = ...;
> extent.end = ...;
> if (VIR_APPEND_ELEMENT_INPLACE(vol->source.extents, vol->source.nextent,
>extent) < 0)
> goto cleanup;
> }
> 
> then the cuurent "ugly" approach  vol->source.extents[vol->source.nextent] .
> 

Other than the VIR_REALLOC_N my current copy uses this method without
the _INPLACE macro.


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-02-01 Thread Pavel Hrdina 
On Mon, Feb 01, 2016 at 09:27:19AM -0500, John Ferlan wrote:
> 
> 
> On 02/01/2016 08:11 AM, Pavel Hrdina wrote:
> > On Mon, Feb 01, 2016 at 07:27:54AM -0500, John Ferlan wrote:
> >>> 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?
> > 
> > Yes, I would rather use the regex as it's more robust.  This was just to
> > demonstrate the comma in device name.  I'm not sure, whether it's possible 
> > to
> > have some storage device with comma in its name, but why to remove code, 
> > that's
> > prepared to handle also this one corner case?
> > 
> 
> So by removing the virStringSplitCount do you have any suggestions to
> get the 'nextents' value?
> 
> This patch also removed the 'stripes' value which is supposed to be only
> valid on striped and mirror lv's, although it has been found valid for
> 'linear', 'thin-pool', and 'thin'.
> 
> I'm back at square 1 with one adjustment:
> 
> nextents = 1;
> if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED) ||
> STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_MIRROR)) {
> if (virStrToLong_i(groups[5], NULL, 10, ) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>_("malformed volume extent stripes value"));
> goto cleanup;
> }
> }

Yes, this is much better.  Parse the nextents only for segtypes where it makes
sense.  And that value will be used to create the regex, like the original code.
The only change would be to cleanup the code.

> 
> where patch 6 adds in the following a the top to avoid the setting of
> any extents data or search through the now possibly empty devices:
> 
> /* If the groups field is NULL, then there's nothing to do */
> if (!groups[3])
> return 0;
> 
> 
> >> -
> >> -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(, 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).
> > 
> > Well, the benefit of VIR_APPEND_ELEMENT macros is that it does some error
> > checking and also memset the original element to 0.  Like I wrote, it's not
> > different but the code is cleaner and easier to read and since we have those
> > macros, why don't use them?
> > 
> 
> I find the REALLOC_N more readable, you find APPEND more readable. Like
> I noted already it's 50/50 in the code REALLOC vs. APPEND. I'm fine with
> using the APPEND option.

That's because a lot of code using REALLOC_N was written before those macros
existed.  Those macros does for us memory movement, cleanups, increasing the
counter and error checking.

> 
> > I personally like this approach:
> > 
> > virStorageVolSourceExtent extent = { NULL, 0, 0 }; // or use memset to 0
> > 
> > if (VIR_REALLOC_N(vol->source.extents,

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

2016-01-29 Thread Pavel Hrdina 
On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote:

[...]

> Signed-off-by: John Ferlan 
> ---
>  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, ) < 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, )))
> +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.

>  /* Allocate and fill in extents information */
>  if (VIR_REALLOC_N(vol->source.extents,
>vol->source.nextent + nextents) < 0)
>  goto cleanup;
> +vol->source.nextent += nextents;

I've also mentioned, that this could be done using VIR_APPEND_ELEMENT by
dropping this pre-allocation and ... [1]

>  
> -if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
> +if (virStrToLong_ull(groups[5], NULL, 10, ) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("malformed volume extent length value"));
>  goto cleanup;
>  }
>  
> -if (virStrToLong_ull(groups[7], NULL, 10, ) < 0) {
> +if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("malformed volume extent size value"));
>  goto cleanup;
>  }
>  
> -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));
> -}

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.

[...]

> -
> -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().

>  }
> -
>  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?

Pavel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-01-29 Thread John Ferlan 


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 
>> ---
>>  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, ) < 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, )))
>> +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].

I went back to the archives for the patches that Osier posted in Oct
2011 and Sep 2011. The reviews back then indicated concern over not
parsing the comma separated devices list... That is - the whole impetus
for making that change was that up to that point, this code used a comma
for a separator when generating the output. The initial change was to
just use "#", but then that change "grew" because it was pointed out
that the devices wouldn't be properly displayed in vol-dumpxml - or at
least separate devices would be listed.


>>  /* Allocate and fill in extents information */
>>  if (VIR_REALLOC_N(vol->source.extents,
>>vol->source.nextent + nextents) < 0)
>>  goto cleanup;
>> +vol->source.nextent += nextents;
> 
> I've also mentioned, that this could be done using VIR_APPEND_ELEMENT by
> dropping this pre-allocation and ... [1]
> 

According to cscope, usage is 50/50. "Many" of the uses have constructs
such as:

   size_t nnames;
   virStructNamePtr *names;

while the extents are defined as :

int nextent;
virStorageVolSourceExtentPtr extents;

>>  
>> -if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
>> +if (virStrToLong_ull(groups[5], NULL, 10, ) < 0) {
>>  virReportError(VIR_ERR_INTERNAL_ERROR,
>> "%s", _("malformed volume extent length value"));
>>  goto cleanup;
>>  }
>>  
>> -if (virStrToLong_ull(groups[7], NULL, 10, ) < 0) {
>> +if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
>>  virReportError(VIR_ERR_INTERNAL_ERROR,
>> "%s", _("malformed volume extent size value"));
>>  goto cleanup;
>>  }
>>  
>> -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

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

2016-01-28 Thread John Ferlan 
Amongst many other changes, commit id '82c1740a' added a display of the
lvs -o 'stripes' field in order to be able to determine the number of
extents listed in the 'devices' output. This was then used to generate
a regex in order to parse the devices string. As part of the generation
of that regex and various other allocations was adding a comma ","
separator for each regex. In essance quite a bit of code in order to
parse a comma separated set of strings having a specific format.

Furthermore, the 'stripes' field is described as the "Number of stripes
or mirror legs"; however, the setting of the 'nextents' value to something
other than 1 was masked behind comparing the output of the lvs -o 'segtype'
field to "striped". Thus, for a "mirror" volume (or today for raid segtypes)
the code would assume only 1 extent (or device) in the list.  This would
lead to output in a vol-dumpxml of:

  

  

   

This patch removes all the regex/regcomp logic in favor of a simpler
mechanism of splitting the devices (groups[3]) output by the comma ","
separator. Then once split, each output string is further parsed
extracting out the parenthesized starting extent number. This is
then all stored in the volume extents array.

The resulting mirror output is then:

  

  


  

  

Although used now, the 'segtype' field is kept for usage by a
future patch.

Signed-off-by: John Ferlan 
---
 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, ) < 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, )))
+goto cleanup;
 
 /* Allocate and fill in extents information */
 if (VIR_REALLOC_N(vol->source.extents,
   vol->source.nextent + nextents) < 0)
 goto cleanup;
+vol->source.nextent += nextents;
 
-if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
+if (virStrToLong_ull(groups[5], NULL, 10, ) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("malformed volume extent length value"));
 goto cleanup;
 }
 
-if (virStrToLong_ull(groups[7], NULL, 10, ) < 0) {
+if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("malformed volume extent size value"));
 goto cleanup;
 }
 
-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));
-}
-
-if (VIR_ALLOC(reg) < 0)
-goto cleanup;
-
-/* Each extent has a "path:offset" pair, and vars[0] will
- * be the whole matched string.
+/* The lvs 'devices' field is described as "Underlying devices used
+ * with starting extent numbers." - The format is a "path" element
+ * followed by a "(#)", such as:
+ *
+ */dev/hda2(0) <--- linear segtype
+ */dev/sdc1(10240),/dev/sdd1(0)<--- striped segtype
+ *test_mirror_rimage_0(0),test_mirror_rimage_1(0)   <-- mirror/raid
+ *  segtype
+