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

[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 <jferlan at 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