Rather than preallocating a set number of elements, then walking through the extents and adjusting the specific element in place, use the APPEND macros to handle that chore. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/storage_conf.h | 2 +- src/storage/storage_backend_logical.c | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index f1dc62b..31b45be 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -50,7 +50,7 @@ struct _virStorageVolSourceExtent { typedef struct _virStorageVolSource virStorageVolSource; typedef virStorageVolSource *virStorageVolSourcePtr; struct _virStorageVolSource { - int nextent; + size_t nextent; virStorageVolSourceExtentPtr extents; int partType; /* virStorageVolTypeDisk, only used by disk diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 7c05b6a..bb02d8a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -84,6 +84,9 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, size_t i; int err, nvars; unsigned long long offset, size, length; + virStorageVolSourceExtent extent; + + memset(&extent, 0, sizeof(extent)); nextents = 1; if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) { @@ -94,11 +97,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, } } - /* Allocate and fill in extents information */ - if (VIR_REALLOC_N(vol->source.extents, - vol->source.nextent + nextents) < 0) - goto cleanup; - if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent length value")); @@ -162,7 +160,7 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, len = vars[j].rm_eo - vars[j].rm_so; p[vars[j].rm_eo] = '\0'; - if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path, + if (VIR_STRNDUP(extent.path, p + vars[j].rm_so, len) < 0) goto cleanup; @@ -176,12 +174,13 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, VIR_FREE(offset_str); goto cleanup; } - VIR_FREE(offset_str); + extent.start = offset * size; + extent.end = (offset * size) + length; - vol->source.extents[vol->source.nextent].start = offset * size; - vol->source.extents[vol->source.nextent].end = (offset * size) + length; - vol->source.nextent++; + if (VIR_APPEND_ELEMENT(vol->source.extents, vol->source.nextent, + extent) < 0) + goto cleanup; } ret = 0; @@ -190,6 +189,7 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, VIR_FREE(regex); VIR_FREE(reg); VIR_FREE(vars); + VIR_FREE(extent.path); return ret; } -- 2.5.0

The 'stripes' value is described as the "Number of stripes or mirrors in a logical volume". So add "mirror" and anything that starts with "raid" to the list of segtypes that can have an 'nextents' value greater than one. Use of raid segtypes (raid1, raid4, raid5*, raid6*, and raid10) is favored over mirror in more recent lvm code. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_logical.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index bb02d8a..cd0fec0 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -65,6 +65,8 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_MIRROR "mirror" +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_RAID "raid" struct virStorageBackendLogicalPoolVolData { virStoragePoolObjPtr pool; @@ -88,8 +90,17 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, memset(&extent, 0, sizeof(extent)); + /* Assume 1 extent (the regex for 'devices' is "(\\S+)") and only + * check the 'stripes' field if we have a striped, mirror, or one of + * the raid (raid1, raid4, raid5*, raid6*, or raid10) segtypes in which + * case the stripes field will denote the number of lv's within the + * 'devices' field in order to generate the proper regex to decode + * the field + */ nextents = 1; - if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) { + if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED) || + STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_MIRROR) || + STRPREFIX(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_RAID)) { if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent stripes value")); -- 2.5.0

Rather than a loop reallocating space to build the regex, just allocate it once up front, then if there's more than 1 nextent, append a comma and another regex_unit string. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_logical.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index cd0fec0..eb22fd0 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -120,12 +120,11 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, goto cleanup; } - if (VIR_STRDUP(regex, regex_unit) < 0) + /* Allocate space for 'nextents' regex_unit strings plus a comma for each */ + if (VIR_ALLOC_N(regex, nextents * (strlen(regex_unit) + 1) + 1) < 0) goto cleanup; - + strncat(regex, regex_unit, strlen(regex_unit)); 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)); -- 2.5.0

Modify the regex for the 'devices' (a/k/a 'extents') from "(\\S+)" (e.g., 1 or more) to "(\\S*)" (e.g., zero or more). Then for any "thin" lv's found, mark the volume as a sparse volume so that the volume wipe algorithm doesn't work. Since a "thin" segtype has no devices, this will result in any "thin" lv part of some thin-pool within a volume group used as a libvirt pool to be displayed as a possible volume to use. NB: Based on a proposal authored by Joe Harvell <joe.harvell(a)tekcomms.com&gt;, but with much intervening rework, the resulting patch is changed from the original concept. About all that remains is changing the regex and checking for NULL/empty field during parse. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_logical.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index eb22fd0..601a896 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -88,9 +88,13 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, unsigned long long offset, size, length; virStorageVolSourceExtent extent; + /* If the devices field is NULL or empty, then there's nothing to do */ + if (!groups[3] || !*groups[3]) + return 0; + memset(&extent, 0, sizeof(extent)); - /* Assume 1 extent (the regex for 'devices' is "(\\S+)") and only + /* Assume 1 extent (since we checked for NULL or empty above) and only * check the 'stripes' field if we have a striped, mirror, or one of * the raid (raid1, raid4, raid5*, raid6*, or raid10) segtypes in which * case the stripes field will denote the number of lv's within the @@ -257,13 +261,15 @@ virStorageBackendLogicalMakeVol(char **const groups, goto cleanup; } - /* Mark the (s) sparse/snapshot lv, e.g. the lv created using - * the --virtualsize/-V option. We've already ignored the (t)hin - * pool definition. In the manner libvirt defines these, the - * thin pool is hidden to the lvs output, except as the name - * in brackets [] described for the groups[1] (backingStore). + /* Mark the (s) sparse/snapshot or the (V) thin/thin-pool member lv, + * e.g. the lv created using the --virtualsize/-V option to ensure + * the volume wipe algorithm doesn't overwrite sparse/thin volumes. + * We've already ignored the (t)hin pool definition. In the manner + * libvirt defines these, the thin pool is hidden to the lvs output, + * except as the name in brackets [] described for the groups[1] + * (backingStore). */ - if (attrs[0] == 's') + if (attrs[0] == 's' || attrs[0] == 'V') vol->target.sparse = true; /* Skips the backingStore of lv created with "--virtualsize", @@ -342,7 +348,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" }; int vars[] = { 10 -- 2.5.0

On 02/02/2016 03:11 AM, Ján Tomko wrote: OK - so if I create a 5th patch with something like, is that what you'd like to see: +#define VIR_STORAGE_VOL_LOGICAL_PREFIX_REGEX "^\\s*" +#define VIR_STORAGE_VOL_LOGICAL_LV_NAME_REGEX "(\\S+)#" +#define VIR_STORAGE_VOL_LOGICAL_ORIGIN_REGEX "(\\S*)#" +#define VIR_STORAGE_VOL_LOGICAL_UUID_REGEX "(\\S+)#" +#define VIR_STORAGE_VOL_LOGICAL_DEVICES_REGEX "(\\S*)#" +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_REGEX "(\\S+)#" +#define VIR_STORAGE_VOL_LOGICAL_STRIPES_REGEX "([0-9]+)#" +#define VIR_STORAGE_VOL_LOGICAL_SEG_SIZE_REGEX "(\\S+)#" +#define VIR_STORAGE_VOL_LOGICAL_VG_EXTENT_SIZE_REGEX "([0-9]+)#" +#define VIR_STORAGE_VOL_LOGICAL_SIZE_REGEX "([0-9]+)#" +#define VIR_STORAGE_VOL_LOGICAL_LV_ATTR_REGEX "(\\S+)#" +#define VIR_STORAGE_VOL_LOGICAL_SUFFIX_REGEX "?\\s*$" + +#define VIR_STORAGE_VOL_LOGICAL_REGEX_COUNT 10 +#define VIR_STORAGE_VOL_LOGICAL_REGEX \ + VIR_STORAGE_VOL_LOGICAL_PREFIX_REGEX \ + VIR_STORAGE_VOL_LOGICAL_LV_NAME_REGEX \ + VIR_STORAGE_VOL_LOGICAL_ORIGIN_REGEX \ + VIR_STORAGE_VOL_LOGICAL_UUID_REGEX \ + VIR_STORAGE_VOL_LOGICAL_DEVICES_REGEX \ + VIR_STORAGE_VOL_LOGICAL_SEGTYPE_REGEX \ + VIR_STORAGE_VOL_LOGICAL_STRIPES_REGEX \ + VIR_STORAGE_VOL_LOGICAL_SEG_SIZE_REGEX \ + VIR_STORAGE_VOL_LOGICAL_VG_EXTENT_SIZE_REGEX \ + VIR_STORAGE_VOL_LOGICAL_SIZE_REGEX \ + VIR_STORAGE_VOL_LOGICAL_LV_ATTR_REGEX \ + VIR_STORAGE_VOL_LOGICAL_SUFFIX_REGEX + and const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" + VIR_STORAGE_VOL_LOGICAL_REGEX }; int vars[] = { - 10 + VIR_STORAGE_VOL_LOGICAL_REGEX_COUNT };