4:44 p.m.
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
4:44 p.m.
New subject: [libvirt] [PATCH v2 1/6] logical: Create helper virStorageBackendLogicalParseVolExtents
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(a)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
9:32 a.m.
New subject: [libvirt] [PATCH v2 1/6] logical: Create helper virStorageBackendLogicalParseVolExtents
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(a)redhat.com>
---
src/storage/storage_backend_logical.c | 208 ++++++++++++++++++----------------
1 file changed, 113 insertions(+), 95 deletions(-)
ACK
4:44 p.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
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(a)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
10:07 a.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote:
[...]
Signed-off-by: John Ferlan <jferlan(a)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
11:50 a.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
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(a)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
3:48 a.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
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(a)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
6:27 a.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
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(a)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
7:11 a.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
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
8:27 a.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
On 02/01/2016 08:11 AM, Pavel Hrdina wrote:
On Mon, Feb 01, 2016 at 07:27:54AM -0500, John Ferlan wrote:
>> That's not true, regex is greedy and it tries to match as much as possible:
>>
>> https://regex101.com/r/lS9tZ5/1
>>
>> Like I said, the regex is more robust and parse the string correctly event with
>> comma as part of a device name, for example "/dev/sda,1".
>>
>
> So do you want the regex back in? Doesn't really matter to me either
> way. I am curious though about what construct is "/dev/sda,1" - that
> is, how is it used?
Yes, I would rather use the regex as it's more robust. This was just to
demonstrate the comma in device name. I'm not sure, whether it's possible to
have some storage device with comma in its name, but why to remove code, that's
prepared to handle also this one corner case?
So by removing the virStringSplitCount do you have any suggestions to
get the 'nextents' value?
This patch also removed the 'stripes' value which is supposed to be only
valid on striped and mirror lv's, although it has been found valid for
'linear', 'thin-pool', and 'thin'.
I'm back at square 1 with one adjustment:
nextents = 1;
if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED) ||
STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_MIRROR)) {
if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed volume extent stripes value"));
goto cleanup;
}
}
where patch 6 adds in the following a the top to avoid the setting of
any extents data or search through the now possibly empty devices:
/* If the groups field is NULL, then there's nothing to do */
if (!groups[3])
return 0;
>>>>> -
>>>>> - vol->source.extents[vol->source.nextent].start =
offset * size;
>>>>> - vol->source.extents[vol->source.nextent].end = (offset
* size) + length;
>>>>> - vol->source.nextent++;
>>>>> + vol->source.extents[i].start = offset * size;
>>>>> + vol->source.extents[i].end = (offset * size) + length;
>>>>
>>>> [1] ... there you would call VIR_APPEND_ELEMENT().
>>>>
>>>
>>> I don't find the whole VIR_APPEND_ELEMENT option as clean... But
I'll
>>> go with it ...
>>>
>>> Usage will require a change to src/conf/storage_conf.h in order to
>>> change 'nextent' to a 'size_t' (from an 'int') -
compiler complains
>>> otherwise.
>>>
>>> Then remove the VIR_REALLOC_N
>>>
>>> Add a memset(&extent, 0, sizeof(extent)); prior to the for loop and one
>>> at the end of the for loop after the successful VIR_APPEND_ELEMENT
>>
>> You don't need to add one after the successful VIR_APPEND_ELEMENT, it does
that
>> for us. There is a different version called VIR_APPEND_ELEMENT_COPY to not
>> clean the original element.
>>
>> In this case, where we know the number of elements, we can pre-allocate the
>> array and use VIR_APPEND_ELEMENT_INSERT which requires the array to be
>> allocated.
>>
>
> In which case VIR_REALLOC_N of some known sized array and then accessing
> each element isn't that much different. Since the more common case is
> "1" extent the difference between the two is nothing, especially since
> in the long run the APPEND macros call virReallocN. I perhaps would feel
> different if I didn't know the value of 'nextents' or this code was
> adding 1 extent at a time...
>
> Ironically the two places that use VIR_APPEND_ELEMENT_INPLACE have
> callers that will use VIR_REALLOC_N (qemuDomainAttachRNGDevice and in a
> much more convoluted path via virDomainChrPreAlloc).
Well, the benefit of VIR_APPEND_ELEMENT macros is that it does some error
checking and also memset the original element to 0. Like I wrote, it's not
different but the code is cleaner and easier to read and since we have those
macros, why don't use them?
I find the REALLOC_N more readable, you find APPEND more readable. Like
I noted already it's 50/50 in the code REALLOC vs. APPEND. I'm fine with
using the APPEND option.
I personally like this approach:
virStorageVolSourceExtent extent = { NULL, 0, 0 }; // or use memset to 0
if (VIR_REALLOC_N(vol->source.extents, vol->source.nextent + nextents) < 0)
goto cleanup;
for (i = 0; i < nextents; i++) {
...
if (VIR_STRNDUP(extent.path, ...) < 0)
goto cleanup;
extent.start = ...;
extent.end = ...;
if (VIR_APPEND_ELEMENT_INPLACE(vol->source.extents, vol->source.nextent,
extent) < 0)
goto cleanup;
}
then the cuurent "ugly" approach
vol->source.extents[vol->source.nextent] .
Other than the VIR_REALLOC_N my current copy uses this method without
the _INPLACE macro.
John
9:27 a.m.
New subject: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:44 p.m.
New subject: [libvirt] [PATCH v2 3/6] logical: Search for a segtype of "thin" and mark lv as sparse
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(a)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
6:09 a.m.
New subject: [libvirt] [PATCH v2 3/6] logical: Search for a segtype of "thin" and mark lv as sparse
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(a)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
7:26 a.m.
New subject: [libvirt] [PATCH v2 3/6] logical: Search for a segtype of "thin" and mark lv as sparse
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(a)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
4:44 p.m.
New subject: [libvirt] [PATCH v2 4/6] vol: Add thin_pool to _virStorageVolSource
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(a)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
6:45 a.m.
New subject: [libvirt] [PATCH v2 4/6] vol: Add thin_pool to _virStorageVolSource
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(a)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 */
};
8 a.m.
New subject: [libvirt] [PATCH v2 4/6] vol: Add thin_pool to _virStorageVolSource
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(a)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 */
> };
9:55 a.m.
New subject: [libvirt] [PATCH v2 4/6] vol: Add thin_pool to _virStorageVolSource
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:52 p.m.
New subject: [libvirt] [PATCH v2 4/6] vol: Add thin_pool to _virStorageVolSource
> <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
3:41 a.m.
New subject: [libvirt] [PATCH v2 4/6] vol: Add thin_pool to _virStorageVolSource
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
4:44 p.m.
New subject: [libvirt] [PATCH v2 5/6] logical: Add capability to get the thin pool name
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(a)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
7:17 a.m.
New subject: [libvirt] [PATCH v2 5/6] logical: Add capability to get the thin pool name
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(a)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
4:44 p.m.
New subject: [libvirt] [PATCH v2 6/6] logical: Display thin lv's found in a libvirt managed volume group
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(a)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(a)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
7:25 a.m.
New subject: [libvirt] [PATCH v2 6/6] logical: Display thin lv's found in a libvirt managed volume group
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(a)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(a)redhat.com>
---
src/storage/storage_backend_logical.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
This is also ACKable
Pavel
3202
days inactive
3207
days old
23 comments
2 participants
participants (2)
-
John Ferlan -
Pavel Hrdina