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".
> >> - 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list