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

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, &nextents) < 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(&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 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list