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

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

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