[libvirt] [PATCH v2 0/6] Some logical pool/volume changes
v1: http://www.redhat.com/archives/libvir-list/2016-January/msg01023.html Differences to v1: Patch 1 is the former patch 2 with the following adjuments: - Change helper name to be virStorageBackendLogicalParseVolExtents - Include all 'extent' parsing needs - including fetch/parse of 'stripes' to determine 'nextents' value, allocation of vol->source.extents, parse of groups[6] to be 'length', parse of groups[7] to be 'size' Patch 2 is new, but replaces some of the concepts from patch 3: - I rototilled the virStorageBackendLogicalParseVolExtents to remove the unnecessary trip through regex/regcomp. - Used virStringSplitCount instread and then parse the resultant string. - Removed the need for the 'stripes' fetch since it really wasn't doing what it was supposed to. - I adjusted the virstringtest to exhibit what is being parsed (it can be removed if you think that's excessive). Patches 3-5 are new - These could be combined. I left them separate to show the process... - Essentially adding a new "source" field to be used to display in the vol-dumpxml output Patch 6 is part of the old patch 4: - Since the old patch 3 isn't necessary - it's essentially gone. This just takes the patch 4 concept of changing the regex, but also applies it to the current code. John Ferlan (6): logical: Create helper virStorageBackendLogicalParseVolExtents logical: Fix parsing of the 'devices' field lvs output logical: Search for a segtype of "thin" and mark lv as sparse vol: Add thin_pool to _virStorageVolSource logical: Add capability to get the thin pool name logical: Display thin lv's found in a libvirt managed volume group docs/formatstorage.html.in | 9 +- src/conf/storage_conf.c | 5 + src/conf/storage_conf.h | 1 + src/storage/storage_backend_logical.c | 221 ++++++++++++++++------------------ tests/virstringtest.c | 11 ++ 5 files changed, 127 insertions(+), 120 deletions(-) -- 2.5.0
Create a helper routine in order to parse any extents information including the extent size, length, and the device string contained within the generated 'lvs' output string. A future patch would then be able to avoid the code more cleanly Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 208 ++++++++++++++++++---------------- 1 file changed, 113 insertions(+), 95 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 944be40..7c05b6a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -72,98 +72,18 @@ struct virStorageBackendLogicalPoolVolData { }; static int -virStorageBackendLogicalMakeVol(char **const groups, - void *opaque) +virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, + char **const groups) { - struct virStorageBackendLogicalPoolVolData *data = opaque; - virStoragePoolObjPtr pool = data->pool; - virStorageVolDefPtr vol = NULL; - bool is_new_vol = false; - unsigned long long offset, size, length; + 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, nextents, nvars, ret = -1; - const char *attrs = groups[9]; - - /* Skip inactive volume */ - if (attrs[4] != 'a') - return 0; - - /* - * Skip thin pools(t). These show up in normal lvs output - * but do not have a corresponding /dev/$vg/$lv device that - * is created by udev. This breaks assumptions in later code. - */ - if (attrs[0] == 't') - return 0; - - /* See if we're only looking for a specific volume */ - if (data->vol != NULL) { - vol = data->vol; - if (STRNEQ(vol->name, groups[0])) - return 0; - } - - /* Or filling in more data on an existing volume */ - if (vol == NULL) - vol = virStorageVolDefFindByName(pool, groups[0]); - - /* Or a completely new volume */ - if (vol == NULL) { - if (VIR_ALLOC(vol) < 0) - return -1; - - is_new_vol = true; - vol->type = VIR_STORAGE_VOL_BLOCK; - - if (VIR_STRDUP(vol->name, groups[0]) < 0) - goto cleanup; - - } - - if (vol->target.path == NULL) { - if (virAsprintf(&vol->target.path, "%s/%s", - pool->def->target.path, vol->name) < 0) - 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). - */ - if (attrs[0] == 's') - vol->target.sparse = true; - - /* Skips the backingStore of lv created with "--virtualsize", - * its original device "/dev/$vgname/$lvname_vorigin" is - * just for lvm internal use, one should never use it. - * - * (lvs outputs "[$lvname_vorigin] for field "origin" if the - * lv is created with "--virtualsize"). - */ - if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] != '[')) { - if (VIR_ALLOC(vol->target.backingStore) < 0) - goto cleanup; - - if (virAsprintf(&vol->target.backingStore->path, "%s/%s", - pool->def->target.path, groups[1]) < 0) - goto cleanup; - - vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; - } - - if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) - goto cleanup; - - if (virStorageBackendUpdateVolInfo(vol, false, - VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) - goto cleanup; + int err, nvars; + unsigned long long offset, size, length; nextents = 1; if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) { @@ -174,7 +94,7 @@ virStorageBackendLogicalMakeVol(char **const groups, } } - /* Finally fill in extents information */ + /* Allocate and fill in extents information */ if (VIR_REALLOC_N(vol->source.extents, vol->source.nextent + nextents) < 0) goto cleanup; @@ -184,18 +104,13 @@ virStorageBackendLogicalMakeVol(char **const groups, "%s", _("malformed volume extent length value")); goto cleanup; } + if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent size value")); goto cleanup; } - if (virStrToLong_ull(groups[8], NULL, 10, &vol->target.allocation) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed volume allocation value")); - goto cleanup; - } - /* Now parse the "devices" field separately */ if (VIR_STRDUP(regex, regex_unit) < 0) goto cleanup; @@ -269,6 +184,112 @@ virStorageBackendLogicalMakeVol(char **const groups, vol->source.nextent++; } + ret = 0; + + cleanup: + VIR_FREE(regex); + VIR_FREE(reg); + VIR_FREE(vars); + return ret; +} + + +static int +virStorageBackendLogicalMakeVol(char **const groups, + void *opaque) +{ + struct virStorageBackendLogicalPoolVolData *data = opaque; + virStoragePoolObjPtr pool = data->pool; + virStorageVolDefPtr vol = NULL; + bool is_new_vol = false; + int ret = -1; + const char *attrs = groups[9]; + + /* Skip inactive volume */ + if (attrs[4] != 'a') + return 0; + + /* + * Skip thin pools(t). These show up in normal lvs output + * but do not have a corresponding /dev/$vg/$lv device that + * is created by udev. This breaks assumptions in later code. + */ + if (attrs[0] == 't') + return 0; + + /* See if we're only looking for a specific volume */ + if (data->vol != NULL) { + vol = data->vol; + if (STRNEQ(vol->name, groups[0])) + return 0; + } + + /* Or filling in more data on an existing volume */ + if (vol == NULL) + vol = virStorageVolDefFindByName(pool, groups[0]); + + /* Or a completely new volume */ + if (vol == NULL) { + if (VIR_ALLOC(vol) < 0) + return -1; + + is_new_vol = true; + vol->type = VIR_STORAGE_VOL_BLOCK; + + if (VIR_STRDUP(vol->name, groups[0]) < 0) + goto cleanup; + + } + + if (vol->target.path == NULL) { + if (virAsprintf(&vol->target.path, "%s/%s", + pool->def->target.path, vol->name) < 0) + 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). + */ + if (attrs[0] == 's') + vol->target.sparse = true; + + /* Skips the backingStore of lv created with "--virtualsize", + * its original device "/dev/$vgname/$lvname_vorigin" is + * just for lvm internal use, one should never use it. + * + * (lvs outputs "[$lvname_vorigin] for field "origin" if the + * lv is created with "--virtualsize"). + */ + if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] != '[')) { + if (VIR_ALLOC(vol->target.backingStore) < 0) + goto cleanup; + + if (virAsprintf(&vol->target.backingStore->path, "%s/%s", + pool->def->target.path, groups[1]) < 0) + goto cleanup; + + vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2; + } + + if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) + goto cleanup; + + if (virStorageBackendUpdateVolInfo(vol, false, + VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) + goto cleanup; + + if (virStrToLong_ull(groups[8], NULL, 10, &vol->target.allocation) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed volume allocation value")); + goto cleanup; + } + + if (virStorageBackendLogicalParseVolExtents(vol, groups) < 0) + goto cleanup; + if (is_new_vol && VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) goto cleanup; @@ -276,9 +297,6 @@ virStorageBackendLogicalMakeVol(char **const groups, ret = 0; cleanup: - VIR_FREE(regex); - VIR_FREE(reg); - VIR_FREE(vars); if (is_new_vol && (ret == -1)) virStorageVolDefFree(vol); return ret; -- 2.5.0
On Thu, Jan 28, 2016 at 05:44:04PM -0500, John Ferlan wrote:
Create a helper routine in order to parse any extents information including the extent size, length, and the device string contained within the generated 'lvs' output string.
A future patch would then be able to avoid the code more cleanly
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 208 ++++++++++++++++++---------------- 1 file changed, 113 insertions(+), 95 deletions(-)
ACK
Amongst many other changes, commit id '82c1740a' added a display of the lvs -o 'stripes' field in order to be able to determine the number of extents listed in the 'devices' output. This was then used to generate a regex in order to parse the devices string. As part of the generation of that regex and various other allocations was adding a comma "," separator for each regex. In essance quite a bit of code in order to parse a comma separated set of strings having a specific format. Furthermore, the 'stripes' field is described as the "Number of stripes or mirror legs"; however, the setting of the 'nextents' value to something other than 1 was masked behind comparing the output of the lvs -o 'segtype' field to "striped". Thus, for a "mirror" volume (or today for raid segtypes) the code would assume only 1 extent (or device) in the list. This would lead to output in a vol-dumpxml of: <source> <device path='lv_test_mirror_rimage_0(0),lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source> This patch removes all the regex/regcomp logic in favor of a simpler mechanism of splitting the devices (groups[3]) output by the comma "," separator. Then once split, each output string is further parsed extracting out the parenthesized starting extent number. This is then all stored in the volume extents array. The resulting mirror output is then: <source> <device path='lv_test_mirror_rimage_0'> <extent start='0' end='33554432'/> </device> <device path='lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source> Although used now, the 'segtype' field is kept for usage by a future patch. Signed-off-by: John Ferlan <jferlan@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; /* Allocate and fill in extents information */ if (VIR_REALLOC_N(vol->source.extents, vol->source.nextent + nextents) < 0) goto cleanup; + vol->source.nextent += nextents; - if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) { + if (virStrToLong_ull(groups[5], NULL, 10, &length) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent length value")); goto cleanup; } - if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) { + if (virStrToLong_ull(groups[6], NULL, 10, &size) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent size value")); goto cleanup; } - 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)); - } - - if (VIR_ALLOC(reg) < 0) - goto cleanup; - - /* Each extent has a "path:offset" pair, and vars[0] will - * be the whole matched string. + /* The lvs 'devices' field is described as "Underlying devices used + * with starting extent numbers." - The format is a "path" element + * followed by a "(#)", such as: + * + * /dev/hda2(0) <--- linear segtype + * /dev/sdc1(10240),/dev/sdd1(0) <--- striped segtype + * test_mirror_rimage_0(0),test_mirror_rimage_1(0) <-- mirror/raid + * segtype + * + * Details of a mirror can be seen via 'lvs -a -o name,devices' */ - nvars = (nextents * 2) + 1; - if (VIR_ALLOC_N(vars, nvars) < 0) - goto cleanup; - - err = regcomp(reg, regex, REG_EXTENDED); - if (err != 0) { - char error[100]; - regerror(err, reg, error, sizeof(error)); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), - error); - goto cleanup; - } - - err = regexec(reg, groups[3], nvars, vars, 0); - regfree(reg); - if (err != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed volume extent devices value")); - goto cleanup; - } - - p = groups[3]; - - /* vars[0] is skipped */ for (i = 0; i < nextents; i++) { - size_t j; - int len; - char *offset_str = NULL; - - j = (i * 2) + 1; - 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, - p + vars[j].rm_so, len) < 0) + if (!(extnum = strchr(extents[i], '(')) || + !(end = strchr(extnum, ')'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed volume extent device entry: '%s'"), + extents[0]); goto cleanup; + } + *extnum++ = '\0'; + *end = '\0'; - len = vars[j + 1].rm_eo - vars[j + 1].rm_so; - if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0) + if (VIR_STRDUP(vol->source.extents[i].path, extents[i]) < 0) goto cleanup; - if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed volume extent offset value")); - VIR_FREE(offset_str); + if (virStrToLong_ull(extnum, NULL, 10, &offset) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed volume extent offset value '(%s)'"), + extnum); goto cleanup; } - VIR_FREE(offset_str); - - 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; } - ret = 0; cleanup: - VIR_FREE(regex); - VIR_FREE(reg); - VIR_FREE(vars); + virStringFreeList(extents); + return ret; } @@ -203,7 +156,7 @@ virStorageBackendLogicalMakeVol(char **const groups, virStorageVolDefPtr vol = NULL; bool is_new_vol = false; int ret = -1; - const char *attrs = groups[9]; + const char *attrs = groups[8]; /* Skip inactive volume */ if (attrs[4] != 'a') @@ -281,7 +234,7 @@ virStorageBackendLogicalMakeVol(char **const groups, VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) goto cleanup; - if (virStrToLong_ull(groups[8], NULL, 10, &vol->target.allocation) < 0) { + if (virStrToLong_ull(groups[7], NULL, 10, &vol->target.allocation) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume allocation value")); goto cleanup; @@ -308,14 +261,14 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, { /* * # lvs --separator # --noheadings --units b --unbuffered --nosuffix --options \ - * "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr" VGNAME + * "lv_name,origin,uuid,devices,segtype,seg_size,vg_extent_size,size,lv_attr" VGNAME * - * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#linear#1#5234491392#33554432#5234491392#-wi-ao - * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#linear#1#1040187392#33554432#1040187392#-wi-ao - * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#linear#1#1073741824#33554432#1073741824#owi-a- - * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#linear#1#2181038080#33554432#2181038080#-wi-a- - * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#linear#1#1040187392#33554432#1040187392#swi-a- - * test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#/dev/sdc1(10240),/dev/sdd1(0)#striped#2#42949672960#4194304#-wi-a- + * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#linear#5234491392#33554432#5234491392#-wi-ao + * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#linear#1040187392#33554432#1040187392#-wi-ao + * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#linear#1073741824#33554432#1073741824#owi-a- + * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#linear#2181038080#33554432#2181038080#-wi-a- + * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#linear#1040187392#33554432#1040187392#swi-a- + * test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#/dev/sdc1(10240),/dev/sdd1(0)#striped#42949672960#4194304#-wi-a- * * Pull out name, origin, & uuid, device, device extent start #, * segment size, extent size, size, attrs @@ -332,10 +285,10 @@ 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+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" }; int vars[] = { - 10 + 9 }; int ret = -1; virCommandPtr cmd; @@ -351,7 +304,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, "--unbuffered", "--nosuffix", "--options", - "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr", + "lv_name,origin,uuid,devices,segtype,seg_size,vg_extent_size,size,lv_attr", pool->def->source.name, NULL); if (virCommandRunRegex(cmd, diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 38d0126..faa3ce9 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -623,6 +623,17 @@ mymain(void) const char *tokens8[] = { "gluster", "rdma", NULL }; TEST_SPLIT("gluster+rdma", "+", 2, tokens8); + const char *tokens9[] = { "/dev/hda2(0)", NULL }; + TEST_SPLIT("/dev/hda2(0)", ",", 1, tokens9); + + const char *tokens10[] = { "/dev/sdc1(10240)", "/dev/sdd1(0)", NULL }; + TEST_SPLIT("/dev/sdc1(10240),/dev/sdd1(0)", ",", 2, tokens10); + + const char *tokens11[] = { "lv_test_mirror_rimage_0(0)", + "lv_test_mirror_rimage_1(0)", NULL }; + TEST_SPLIT("lv_test_mirror_rimage_0(0),lv_test_mirror_rimage_1(0)", ",", + 2, tokens11); + if (virtTestRun("strdup", testStrdup, NULL) < 0) ret = -1; -- 2.5.0
On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote: [...]
Signed-off-by: John Ferlan <jferlan@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.
/* Allocate and fill in extents information */ if (VIR_REALLOC_N(vol->source.extents, vol->source.nextent + nextents) < 0) goto cleanup; + vol->source.nextent += nextents;
I've also mentioned, that this could be done using VIR_APPEND_ELEMENT by dropping this pre-allocation and ... [1]
- if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) { + if (virStrToLong_ull(groups[5], NULL, 10, &length) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent length value")); goto cleanup; }
- if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) { + if (virStrToLong_ull(groups[6], NULL, 10, &size) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent size value")); goto cleanup; }
- 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)); - }
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. [...]
- - 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().
} - 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? Pavel
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@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]. I went back to the archives for the patches that Osier posted in Oct 2011 and Sep 2011. The reviews back then indicated concern over not parsing the comma separated devices list... That is - the whole impetus for making that change was that up to that point, this code used a comma for a separator when generating the output. The initial change was to just use "#", but then that change "grew" because it was pointed out that the devices wouldn't be properly displayed in vol-dumpxml - or at least separate devices would be listed.
/* Allocate and fill in extents information */ if (VIR_REALLOC_N(vol->source.extents, vol->source.nextent + nextents) < 0) goto cleanup; + vol->source.nextent += nextents;
I've also mentioned, that this could be done using VIR_APPEND_ELEMENT by dropping this pre-allocation and ... [1]
According to cscope, usage is 50/50. "Many" of the uses have constructs such as: size_t nnames; virStructNamePtr *names; while the extents are defined as : int nextent; virStorageVolSourceExtentPtr extents;
- if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) { + if (virStrToLong_ull(groups[5], NULL, 10, &length) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent length value")); goto cleanup; }
- if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) { + if (virStrToLong_ull(groups[6], NULL, 10, &size) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent size value")); goto cleanup; }
- 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. 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 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.
} - 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... 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. For example, I have in my test environment (line wrapping ahead): # lvs -a -o name,devices vg_test_mirror LV Devices lv_test_mirror lv_test_mirror_rimage_0(0),lv_test_mirror_rimage_1(0) [lv_test_mirror_rimage_0] /dev/sdk(1) [lv_test_mirror_rimage_1] /dev/sdl(1) [lv_test_mirror_rmeta_0] /dev/sdk(0) [lv_test_mirror_rmeta_1] /dev/sdl(0) So mirror uses /dev/sdk and /dev/sdl ... # lvs -a -o name,devices vg_test_stripe LV Devices lv_nostripe /dev/sdi(0) lv_test_stripes /dev/sdi(7),/dev/sdj(0) This one we already can get ... # lvs -a -o name,devices vg_test_thin LV Devices lv_test_snapshot /dev/sdh(0) [lv_test_snapshot_vorigin] lv_test_thin [lvol0_pmspare] /dev/sdh(2) thinpool_lv_test_thin thinpool_lv_test_thin_tdata(0) [thinpool_lv_test_thin_tdata] /dev/sdh(3) [thinpool_lv_test_thin_tmeta] /dev/sdh(8) So thin uses /dev/sdh. It's not a simple ask/parse operation. John
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@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
On 02/01/2016 04:48 AM, Pavel Hrdina wrote:
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@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".
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?
- 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(); } }
if 'nextents == 0', then we shouldn't get this far ;-)
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.
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). John
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
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
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
On Mon, Feb 01, 2016 at 09:27:19AM -0500, John Ferlan wrote:
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; } }
Yes, this is much better. Parse the nextents only for segtypes where it makes sense. And that value will be used to create the regex, like the original code. The only change would be to cleanup the code.
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.
That's because a lot of code using REALLOC_N was written before those macros existed. Those macros does for us memory movement, cleanups, increasing the counter and error checking.
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
For any "thin" lv's, mark the volume as a sparse volume so that the volume wipe algorithm doesn't work. Currently thin lv's are ignored because the regex requires 1 or more 'devices' listed in order to process. However, a future patch will be changing this. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3232c08..a9c6309 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -64,6 +64,8 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, } +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN "thin" + struct virStorageBackendLogicalPoolVolData { virStoragePoolObjPtr pool; virStorageVolDefPtr vol; @@ -201,12 +203,15 @@ virStorageBackendLogicalMakeVol(char **const groups, } /* Mark the (s) sparse/snapshot lv, e.g. the lv created using - * the --virtualsize/-V option. We've already ignored the (t)hin + * the --virtualsize/-V option or a thin segtype as sparse. This + * will make sure the volume wipe algorithm doesn't overwrite + * a 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' || + STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN)) vol->target.sparse = true; /* Skips the backingStore of lv created with "--virtualsize", -- 2.5.0
On Thu, Jan 28, 2016 at 05:44:06PM -0500, John Ferlan wrote:
For any "thin" lv's, mark the volume as a sparse volume so that the volume wipe algorithm doesn't work. Currently thin lv's are ignored because the regex requires 1 or more 'devices' listed in order to process. However, a future patch will be changing this.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3232c08..a9c6309 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -64,6 +64,8 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, }
+#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN "thin" + struct virStorageBackendLogicalPoolVolData { virStoragePoolObjPtr pool; virStorageVolDefPtr vol; @@ -201,12 +203,15 @@ virStorageBackendLogicalMakeVol(char **const groups, }
/* Mark the (s) sparse/snapshot lv, e.g. the lv created using - * the --virtualsize/-V option. We've already ignored the (t)hin + * the --virtualsize/-V option or a thin segtype as sparse. This + * will make sure the volume wipe algorithm doesn't overwrite + * a 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' || + STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN)) vol->target.sparse = true;
Well, I'm not sure about this code. Based on my research, the 's' means snapshot volume and there is another flag 'V' which is thin volume. The comment doesn't seems to be correct. By using --virtualsize/-V it will create snapshot or thin volume based on 'sparse_segtype_default' configuration option from /etc/lvm/lvm.conf. It would be nice to clarify this in the comment for future review. One more thing, why don't use || attrs[0] == 'V' ? Pavel
On 02/01/2016 07:09 AM, Pavel Hrdina wrote:
On Thu, Jan 28, 2016 at 05:44:06PM -0500, John Ferlan wrote:
For any "thin" lv's, mark the volume as a sparse volume so that the volume wipe algorithm doesn't work. Currently thin lv's are ignored because the regex requires 1 or more 'devices' listed in order to process. However, a future patch will be changing this.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3232c08..a9c6309 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -64,6 +64,8 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, }
+#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN "thin" + struct virStorageBackendLogicalPoolVolData { virStoragePoolObjPtr pool; virStorageVolDefPtr vol; @@ -201,12 +203,15 @@ virStorageBackendLogicalMakeVol(char **const groups, }
/* Mark the (s) sparse/snapshot lv, e.g. the lv created using - * the --virtualsize/-V option. We've already ignored the (t)hin + * the --virtualsize/-V option or a thin segtype as sparse. This + * will make sure the volume wipe algorithm doesn't overwrite + * a 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' || + STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN)) vol->target.sparse = true;
Well, I'm not sure about this code. Based on my research, the 's' means snapshot volume and there is another flag 'V' which is thin volume. The comment doesn't seems to be correct. By using --virtualsize/-V it will create snapshot or thin volume based on 'sparse_segtype_default' configuration option from /etc/lvm/lvm.conf. It would be nice to clarify this in the comment for future review. One more thing, why don't use || attrs[0] == 'V' ?
Yes, 's' is a 'thin snapshot volume', which is different than a 'thin volume in a thin pool'. The '-V'/'--virtualsize' are command line options to lvcreate to generate a sparse volume. Because 'thin pools' became the default at some point in time the libvirt code added a "--type snapshot" to the command line to indicate we wanted what we had been using. Check out commit id 'cafb934d' for some history as well as the details left when I tried to add code to handle 'thin volume as part of a thin_pool': http://www.redhat.com/archives/libvir-list/2014-December/msg00706.html Usage of attrs[0] == 'V' and comparing the segname to "thin" are the same. Without the groups[4] search here though, then 'segname' is unused and could be removed. Doesn't matter either way to me. Just seemed more practical to use groups[4] (e.g. segtype) just in case we needed it in the future. John
A thin lv doesn't have any extents defined - rather it uses a concept of a "thin-pool" in order to describe it's source. In order to allow that to be displayed properly in a future patch, add a new 'thin_pool' name that can be used by a future patch to store the name of the source thin_pool name used by a thin logical volume. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 +++++ src/conf/storage_conf.h | 1 + 2 files changed, 6 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3657dfd..8ceb465 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -322,6 +322,7 @@ virStorageVolDefFree(virStorageVolDefPtr def) for (i = 0; i < def->source.nextent; i++) VIR_FREE(def->source.extents[i].path); VIR_FREE(def->source.extents); + VIR_FREE(def->source.thin_pool); virStorageSourceClear(&def->target); VIR_FREE(def); @@ -1655,6 +1656,10 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virBufferAddLit(&buf, "</device>\n"); } + if (def->source.thin_pool) + virBufferEscapeString(&buf, "<name>%s</name>\n", + def->source.thin_pool); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</source>\n"); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index f1dc62b..7e15a70 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -55,6 +55,7 @@ struct _virStorageVolSource { int partType; /* virStorageVolTypeDisk, only used by disk * backend for partition type creation */ + char *thin_pool; /* Used to print/dumpxml the thin pool name */ }; -- 2.5.0
On Thu, Jan 28, 2016 at 05:44:07PM -0500, John Ferlan wrote:
A thin lv doesn't have any extents defined - rather it uses a concept of a "thin-pool" in order to describe it's source. In order to allow that to be displayed properly in a future patch, add a new 'thin_pool' name that can be used by a future patch to store the name of the source thin_pool name used by a thin logical volume.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 +++++ src/conf/storage_conf.h | 1 + 2 files changed, 6 insertions(+)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3657dfd..8ceb465 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -322,6 +322,7 @@ virStorageVolDefFree(virStorageVolDefPtr def) for (i = 0; i < def->source.nextent; i++) VIR_FREE(def->source.extents[i].path); VIR_FREE(def->source.extents); + VIR_FREE(def->source.thin_pool);
virStorageSourceClear(&def->target); VIR_FREE(def); @@ -1655,6 +1656,10 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virBufferAddLit(&buf, "</device>\n"); }
+ if (def->source.thin_pool) + virBufferEscapeString(&buf, "<name>%s</name>\n", + def->source.thin_pool);
I'm not sure about the element to be called "name". I would suggest something like <thinpool name=''/>. Otherwise it looks good. Pavel
+ virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</source>\n");
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index f1dc62b..7e15a70 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -55,6 +55,7 @@ struct _virStorageVolSource {
int partType; /* virStorageVolTypeDisk, only used by disk * backend for partition type creation */ + char *thin_pool; /* Used to print/dumpxml the thin pool name */ };
On 02/01/2016 07:45 AM, Pavel Hrdina wrote:
On Thu, Jan 28, 2016 at 05:44:07PM -0500, John Ferlan wrote:
A thin lv doesn't have any extents defined - rather it uses a concept of a "thin-pool" in order to describe it's source. In order to allow that to be displayed properly in a future patch, add a new 'thin_pool' name that can be used by a future patch to store the name of the source thin_pool name used by a thin logical volume.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 +++++ src/conf/storage_conf.h | 1 + 2 files changed, 6 insertions(+)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3657dfd..8ceb465 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -322,6 +322,7 @@ virStorageVolDefFree(virStorageVolDefPtr def) for (i = 0; i < def->source.nextent; i++) VIR_FREE(def->source.extents[i].path); VIR_FREE(def->source.extents); + VIR_FREE(def->source.thin_pool);
virStorageSourceClear(&def->target); VIR_FREE(def); @@ -1655,6 +1656,10 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virBufferAddLit(&buf, "</device>\n"); }
+ if (def->source.thin_pool) + virBufferEscapeString(&buf, "<name>%s</name>\n", + def->source.thin_pool);
I'm not sure about the element to be called "name". I would suggest something like <thinpool name=''/>. Otherwise it looks good.
I used <name> because it would be hierarchically similar to <pool>...<source>...<name>... That is <volume type='block'>...<source>...<name> Although this does remind me, storagevol.rng needs to be updated seeing as <device path='%s'> <extent start=### end=###/> is defined. Alternatively, would the following work? <volume type='block'> ... <source> <name>%s</name> <format type='thin_pool'> </source> ... </volume> Trying to forward think if/when a vol.xml was used as input... John
Pavel
+ virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</source>\n");
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index f1dc62b..7e15a70 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -55,6 +55,7 @@ struct _virStorageVolSource {
int partType; /* virStorageVolTypeDisk, only used by disk * backend for partition type creation */ + char *thin_pool; /* Used to print/dumpxml the thin pool name */ };
On Mon, Feb 01, 2016 at 09:00:51AM -0500, John Ferlan wrote:
On 02/01/2016 07:45 AM, Pavel Hrdina wrote:
On Thu, Jan 28, 2016 at 05:44:07PM -0500, John Ferlan wrote:
A thin lv doesn't have any extents defined - rather it uses a concept of a "thin-pool" in order to describe it's source. In order to allow that to be displayed properly in a future patch, add a new 'thin_pool' name that can be used by a future patch to store the name of the source thin_pool name used by a thin logical volume.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 +++++ src/conf/storage_conf.h | 1 + 2 files changed, 6 insertions(+)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3657dfd..8ceb465 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -322,6 +322,7 @@ virStorageVolDefFree(virStorageVolDefPtr def) for (i = 0; i < def->source.nextent; i++) VIR_FREE(def->source.extents[i].path); VIR_FREE(def->source.extents); + VIR_FREE(def->source.thin_pool);
virStorageSourceClear(&def->target); VIR_FREE(def); @@ -1655,6 +1656,10 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virBufferAddLit(&buf, "</device>\n"); }
+ if (def->source.thin_pool) + virBufferEscapeString(&buf, "<name>%s</name>\n", + def->source.thin_pool);
I'm not sure about the element to be called "name". I would suggest something like <thinpool name=''/>. Otherwise it looks good.
I used <name> because it would be hierarchically similar to <pool>...<source>...<name>... That is <volume type='block'>...<source>...<name>
Although this does remind me, storagevol.rng needs to be updated seeing as <device path='%s'> <extent start=### end=###/> is defined.
Alternatively, would the following work?
<volume type='block'> ... <source> <name>%s</name> <format type='thin_pool'> </source> ... </volume>
Trying to forward think if/when a vol.xml was used as input...
Maybe we should stop right here and handle the thin pool/volumes in a separate patch series. It would be best to create a new libvirt pool type that will be dedicated for lvm thin pools and add a proper support for thin pools. Pavel
John
Pavel
+ virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</source>\n");
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index f1dc62b..7e15a70 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -55,6 +55,7 @@ struct _virStorageVolSource {
int partType; /* virStorageVolTypeDisk, only used by disk * backend for partition type creation */ + char *thin_pool; /* Used to print/dumpxml the thin pool name */ };
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
<volume type='block'> ... <source> <name>%s</name> <format type='thin_pool'> </source> ... </volume>
Trying to forward think if/when a vol.xml was used as input...
Maybe we should stop right here and handle the thin pool/volumes in a separate patch series. It would be best to create a new libvirt pool type that will be dedicated for lvm thin pools and add a proper support for thin pools.
Without patches 4 & 5 we get the following for virsh vol-dumpxml: <volume ...> ... <source> </source> ... </volume> Which is OK - although it looks a bit strange to me - it isn't the only output that doesn't have <source> defined. Since a thin lv and thin-pool can be placed into any vg and a vg is essentially a libvirt pool, I'm not so sure having a dedicated pool type is necessary. Sure it'd make some things easier, but can we assume every thin volume instance is configured that way? John
On Mon, Feb 01, 2016 at 03:52:12PM -0500, John Ferlan wrote:
<volume type='block'> ... <source> <name>%s</name> <format type='thin_pool'> </source> ... </volume>
Trying to forward think if/when a vol.xml was used as input...
Maybe we should stop right here and handle the thin pool/volumes in a separate patch series. It would be best to create a new libvirt pool type that will be dedicated for lvm thin pools and add a proper support for thin pools.
Without patches 4 & 5 we get the following for virsh vol-dumpxml:
<volume ...> ... <source> </source> ... </volume>
Actually I have a guest with all different types of logical volume and I wasn't able to list those thin lv via libvirt, so I assumed, that we are somehow ignoring them so far. I'll try it again together with your patches.
Which is OK - although it looks a bit strange to me - it isn't the only output that doesn't have <source> defined.
Since a thin lv and thin-pool can be placed into any vg and a vg is essentially a libvirt pool, I'm not so sure having a dedicated pool type is necessary. Sure it'd make some things easier, but can we assume every thin volume instance is configured that way?
Yes, it could be placed in current logical pool, but IMO it would be good to separate those thin pools and lvs from "classic" lv. If someone would like to use thin volumes in libvirt, one will have to configure new pool where source will be desired thin pool. Pavel
Since a future patch will start displaying thin logical volumes for a logical volume group thin-pool, add the ability to display at the name of the thin-pool name. This is done by adding 'pool_lv' to the list of regex's and then when a "thin" lv is found, storing that pool name. The result will end up being the following for a vol-dumpxml: <source> <name>thinpool_test_thin</name> </source> instead of an empty <source> </source> pair. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 9 +++++++-- src/storage/storage_backend_logical.c | 26 ++++++++++++++++---------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 4965a4c..2f4662c 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -566,8 +566,13 @@ <span class="since">Since 0.4.1</span></dd> <dt><code>source</code></dt> <dd>Provides information about the underlying storage allocation - of the volume. This may not be available for some pool types. - <span class="since">Since 0.4.1</span></dd> + of the volume. This is available logical pool types to display + details of logical volume extent information + (<span class="since">Since 0.4.1</span>) + or the name of the name of the thin-pool used by the thin + logical volume + (<span class="since">Since 1.3.2</span>). + </dd> <dt><code>target</code></dt> <dd>Provides information about the representation of the volume on the local host. <span class="since">Since 0.4.1</span></dd> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index a9c6309..b866648 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -239,6 +239,11 @@ virStorageBackendLogicalMakeVol(char **const groups, VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) goto cleanup; + if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN)) { + if (VIR_STRDUP(vol->source.thin_pool, groups[9]) < 0) + goto cleanup; + } + if (virStrToLong_ull(groups[7], NULL, 10, &vol->target.allocation) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume allocation value")); @@ -266,14 +271,15 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, { /* * # lvs --separator # --noheadings --units b --unbuffered --nosuffix --options \ - * "lv_name,origin,uuid,devices,segtype,seg_size,vg_extent_size,size,lv_attr" VGNAME + * "lv_name,origin,uuid,devices,segtype,seg_size,vg_extent_size,size,lv_attr,pool_lv" VGNAME * - * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#linear#5234491392#33554432#5234491392#-wi-ao - * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#linear#1040187392#33554432#1040187392#-wi-ao - * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#linear#1073741824#33554432#1073741824#owi-a- - * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#linear#2181038080#33554432#2181038080#-wi-a- - * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#linear#1040187392#33554432#1040187392#swi-a- - * test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#/dev/sdc1(10240),/dev/sdd1(0)#striped#42949672960#4194304#-wi-a- + * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#linear#5234491392#33554432#5234491392#-wi-ao# + * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#linear#1040187392#33554432#1040187392#-wi-ao# + * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#linear#1073741824#33554432#1073741824#owi-a-# + * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#linear#2181038080#33554432#2181038080#-wi-a-# + * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#linear#1040187392#33554432#1040187392#swi-a-# + * test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#/dev/sdc1(10240),/dev/sdd1(0)#striped#42949672960#4194304#-wi-a-# + * test_thin##9cdaL5-qEyd-pbRn-Ef2B-uU81-WSIE-XUY973##thin#41943040#4194304#41943040#Vwi-a-tz--#thinpool_test_thin * * Pull out name, origin, & uuid, device, device extent start #, * segment size, extent size, size, attrs @@ -290,10 +296,10 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#(\\S*)#?\\s*$" }; int vars[] = { - 9 + 10 }; int ret = -1; virCommandPtr cmd; @@ -309,7 +315,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, "--unbuffered", "--nosuffix", "--options", - "lv_name,origin,uuid,devices,segtype,seg_size,vg_extent_size,size,lv_attr", + "lv_name,origin,uuid,devices,segtype,seg_size,vg_extent_size,size,lv_attr,pool_lv", pool->def->source.name, NULL); if (virCommandRunRegex(cmd, -- 2.5.0
On Thu, Jan 28, 2016 at 05:44:08PM -0500, John Ferlan wrote:
Since a future patch will start displaying thin logical volumes for a logical volume group thin-pool, add the ability to display at the name of the thin-pool name. This is done by adding 'pool_lv' to the list of regex's and then when a "thin" lv is found, storing that pool name.
The result will end up being the following for a vol-dumpxml:
<source> <name>thinpool_test_thin</name> </source>
instead of an empty <source> </source> pair.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 9 +++++++-- src/storage/storage_backend_logical.c | 26 ++++++++++++++++---------- 2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 4965a4c..2f4662c 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -566,8 +566,13 @@ <span class="since">Since 0.4.1</span></dd> <dt><code>source</code></dt> <dd>Provides information about the underlying storage allocation - of the volume. This may not be available for some pool types. - <span class="since">Since 0.4.1</span></dd> + of the volume. This is available logical pool types to display
s/available/available for/
+ details of logical volume extent information + (<span class="since">Since 0.4.1</span>) + or the name of the name of the thin-pool used by the thin + logical volume + (<span class="since">Since 1.3.2</span>). + </dd> <dt><code>target</code></dt> <dd>Provides information about the representation of the volume on the local host. <span class="since">Since 0.4.1</span></dd>
[...] Looks good, but you'll need to update it after we agree on the name of the new element. Pavel
Modify the regex for the 'devices' (a/k/a 'extents') from "(\\S+)" (e.g., 1 or more) to "(\\S*)" (e.g., zero or more). 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@tekcomms.com>, but with much intervening rework, the resulting patch is changed from the original concept. About all that remains is changing the regex. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index b866648..c56961d 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -81,9 +81,12 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, size_t i, nextents = 0; unsigned long long offset, size, length; + /* If the groups field is NULL, then there's nothing to do */ + if (!groups[3]) + return 0; + /* 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. */ + * each out into a parseable string. */ if (!(extents = virStringSplitCount(groups[3], ",", 0, &nextents))) goto cleanup; @@ -296,7 +299,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#(\\S*)#?\\s*$" + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#(\\S*)#?\\s*$" }; int vars[] = { 10 -- 2.5.0
On Thu, Jan 28, 2016 at 05:44:09PM -0500, John Ferlan wrote:
Modify the regex for the 'devices' (a/k/a 'extents') from "(\\S+)" (e.g., 1 or more) to "(\\S*)" (e.g., zero or more).
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@tekcomms.com>, but with much intervening rework, the resulting patch is changed from the original concept. About all that remains is changing the regex.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
This is also ACKable Pavel
participants (2)
-
John Ferlan -
Pavel Hrdina