* src/storage/storage_backend_logical.c: If a logical vol is created as striped. (e.g. --stripes 3), the "device" field of lvs output will have multiple fileds which are seperated by comma. Thus the RE we write in the codes will not work well anymore. E.g. (lvs output for a stripped vol, uses "#" as seperator here): test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#\ /dev/sdc1(10240),/dev/sdd1(0)#42949672960#4194304 The RE we use: const char *regexes[] = { "^\\s*(\\S+),(\\S*),(\\S+),(\\S+)\\((\\S+)\\),(\\S+),([0-9]+),?\\s*$" }; Also the RE doesn't match the "devices" field of striped vol properly, it contains multiple "device path" and "offset". This patch mainly does: 1) Change the seperator into "#" 2) Change the RE for "devices" field from "(\\S+)\\((\\S+)\\)" into "(\\S+)". 3) Add two new options for lvs command, (segtype, stripes) 4) Extend the RE to match the value for the two new fields. 5) Parse the "devices" field seperately in virStorageBackendLogicalMakeVol, multiple "extents" info are generated if the vol is striped. The number of "extents" is equal to the stripes number of the striped vol. A incidental fix: (virStorageBackendLogicalMakeVol) Free "vol" if it's new created and there is error. Demo on striped vol with the patch applied: % virsh vol-dumpxml /dev/test_vg/vol_striped2 <volume> <name>vol_striped2</name> <key>QuWqmn-kIkZ-IATt-67rc-OWEP-1PHX-Cl2ICs</key> <source> <device path='/dev/sda5'> <extent start='79691776' end='88080384'/> </device> <device path='/dev/sda6'> <extent start='62914560' end='71303168'/> </device> </source> <capacity>8388608</capacity> <allocation>8388608</allocation> <target> <path>/dev/test_vg/vol_striped2</path> <permissions> <mode>0660</mode> <owner>0</owner> <group>6</group> <label>system_u:object_r:fixed_disk_device_t:s0</label> </permissions> </target> </volume> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=727474 v1 - v2: v1 just simply changes the seperator into "#". --- src/storage/storage_backend_logical.c | 156 +++++++++++++++++++++++++-------- 1 files changed, 121 insertions(+), 35 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 589f486..dda68fd 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -62,13 +62,22 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, } +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" + static int virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, char **const groups, void *data) { virStorageVolDefPtr vol = NULL; + bool is_new_vol = false; unsigned long long offset, size, length; + const char *regex_unit = "(\\S+)\\((\\S+)\\)"; + char *regex = NULL; + regex_t *reg = NULL; + regmatch_t *vars = NULL; + char *p = NULL; + int i, err, nextents, nvars, ret = -1; /* See if we're only looking for a specific volume */ if (data != NULL) { @@ -88,19 +97,18 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, return -1; } + is_new_vol = true; vol->type = VIR_STORAGE_VOL_BLOCK; if ((vol->name = strdup(groups[0])) == NULL) { virReportOOMError(); - virStorageVolDefFree(vol); - return -1; + goto cleanup; } if (VIR_REALLOC_N(pool->volumes.objs, pool->volumes.count + 1)) { virReportOOMError(); - virStorageVolDefFree(vol); - return -1; + goto cleanup; } pool->volumes.objs[pool->volumes.count++] = vol; } @@ -109,8 +117,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) < 0) { virReportOOMError(); - virStorageVolDefFree(vol); - return -1; + goto cleanup; } } @@ -118,8 +125,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->backingStore.path, "%s/%s", pool->def->target.path, groups[1]) < 0) { virReportOOMError(); - virStorageVolDefFree(vol); - return -1; + goto cleanup; } vol->backingStore.format = VIR_STORAGE_POOL_LOGICAL_LVM2; @@ -128,47 +134,127 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, if (vol->key == NULL && (vol->key = strdup(groups[2])) == NULL) { virReportOOMError(); - return -1; + goto cleanup; } if (virStorageBackendUpdateVolInfo(vol, 1) < 0) - return -1; + goto cleanup; + nextents = 1; + if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) { + if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent stripes value")); + goto cleanup; + } + } /* Finally fill in extents information */ if (VIR_REALLOC_N(vol->source.extents, - vol->source.nextent + 1) < 0) { + vol->source.nextent + nextents) < 0) { virReportOOMError(); - return -1; + goto cleanup; } - if ((vol->source.extents[vol->source.nextent].path = - strdup(groups[3])) == NULL) { + if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed volume extent length value")); + goto cleanup; + } + if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed volume extent size value")); + goto cleanup; + } + + /* Now parse the "devices" feild seperately */ + regex = strdup(regex_unit); + + for (i = 1; i < nextents; i++) { + if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2) < 0) { + virReportOOMError(); + goto cleanup; + } + /* "," is the seperator of "devices" field */ + strcat(regex, ","); + strncat(regex, regex_unit, strlen(regex_unit)); + } + + if (VIR_ALLOC(reg) < 0) { virReportOOMError(); - return -1; + goto cleanup; } - if (virStrToLong_ull(groups[4], NULL, 10, &offset) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed volume extent offset value")); - return -1; + /* Each extent has a "path:offset" pair, and vars[0] will + * be the whole matched string. + */ + nvars = (nextents * 2) + 1; + if (VIR_ALLOC_N(vars, nvars) < 0) { + virReportOOMError(); + goto cleanup; } - if (virStrToLong_ull(groups[5], NULL, 10, &length) < 0) { + + err = regcomp(reg, regex, REG_EXTENDED); + if (err != 0) { + char error[100]; + regerror(err, reg, error, sizeof(error)); virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed volume extent length value")); - return -1; + _("Failed to compile regex %s"), + error); + goto cleanup; } - if (virStrToLong_ull(groups[6], NULL, 10, &size) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed volume extent size value")); - return -1; + + if (regexec(reg, groups[3], nvars, vars, 0) != 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent devices value")); + goto cleanup; } - vol->source.extents[vol->source.nextent].start = offset * size; - vol->source.extents[vol->source.nextent].end = (offset * size) + length; - vol->source.nextent++; + p = groups[3]; - return 0; + /* vars[0] is skipped */ + for (i = 0; i < nextents; i++) { + int j, len; + const char *offset_str = NULL; + + j = (i * 2) + 1; + len = vars[j].rm_eo - vars[j].rm_so; + p[vars[j].rm_eo] = '\0'; + + if ((vol->source.extents[vol->source.nextent].path = + strndup(p + vars[j].rm_so, len)) == NULL) { + virReportOOMError(); + goto cleanup; + } + + len = vars[j + 1].rm_eo - vars[j + 1].rm_so; + if (!(offset_str = strndup(p + vars[j + 1].rm_so, len))) { + virReportOOMError(); + goto cleanup; + } + + if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent offset value")); + 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++; + } + + ret = 0; + +cleanup: + VIR_FREE(regex); + VIR_FREE(reg); + VIR_FREE(vars); + if (is_new_vol && (ret == -1)) + virStorageVolDefFree(vol); + return ret; } static int @@ -193,15 +279,15 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * not a suitable separator (rhbz 470693). */ const char *regexes[] = { - "^\\s*(\\S+),(\\S*),(\\S+),(\\S+)\\((\\S+)\\),(\\S+),([0-9]+),?\\s*$" + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#?\\s*$"

}; int vars[] = { - 7 + 8 }; const char *prog[] = { - LVS, "--separator", ",", "--noheadings", "--units", "b", + LVS, "--separator", "#", "--noheadings", "--units", "b", "--unbuffered", "--nosuffix", "--options", - "lv_name,origin,uuid,devices,seg_size,vg_extent_size", + "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size", pool->def->source.name, NULL };

On Mon, Oct 10, 2011 at 12:28:04PM +0800, Osier Yang wrote: > * src/storage/storage_backend_logical.c: > > If a logical vol is created as striped. (e.g. --stripes 3), > the "device" field of lvs output will have multiple fileds which are > seperated by comma. Thus the RE we write in the codes will not > work well anymore. E.g. (lvs output for a stripped vol, uses "#" as > seperator here): > > test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#\ > /dev/sdc1(10240),/dev/sdd1(0)#42949672960#4194304 > > The RE we use: > > const char *regexes[] = { > "^\\s*(\\S+),(\\S*),(\\S+),(\\S+)\\((\\S+)\\),(\\S+),([0-9]+),?\\s*$" > }; > > Also the RE doesn't match the "devices" field of striped vol properly, > it contains multiple "device path" and "offset". > > This patch mainly does: > 1) Change the seperator into "#" > 2) Change the RE for "devices" field from "(\\S+)\\((\\S+)\\)" > into "(\\S+)". > 3) Add two new options for lvs command, (segtype, stripes) > 4) Extend the RE to match the value for the two new fields. > 5) Parse the "devices" field seperately in virStorageBackendLogicalMakeVol, > multiple "extents" info are generated if the vol is striped. The > number of "extents" is equal to the stripes number of the striped vol. > > A incidental fix: (virStorageBackendLogicalMakeVol) > Free "vol" if it's new created and there is error. > > Demo on striped vol with the patch applied: > > % virsh vol-dumpxml /dev/test_vg/vol_striped2 > <volume> > <name>vol_striped2</name> > <key>QuWqmn-kIkZ-IATt-67rc-OWEP-1PHX-Cl2ICs</key> > <source> > <device path='/dev/sda5'> > <extent start='79691776' end='88080384'/> > </device> > <device path='/dev/sda6'> > <extent start='62914560' end='71303168'/> > </device> > </source> > <capacity>8388608</capacity> > <allocation>8388608</allocation> > <target> > <path>/dev/test_vg/vol_striped2</path> > <permissions> > <mode>0660</mode> > <owner>0</owner> > <group>6</group> > <label>system_u:object_r:fixed_disk_device_t:s0</label> > </permissions> > </target> > </volume> > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=727474 > > v1 - v2: > v1 just simply changes the seperator into "#". > > --- > src/storage/storage_backend_logical.c | 156 +++++++++++++++++++++++++-------- > 1 files changed, 121 insertions(+), 35 deletions(-) > > diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c > index 589f486..dda68fd 100644 > --- a/src/storage/storage_backend_logical.c > +++ b/src/storage/storage_backend_logical.c > @@ -62,13 +62,22 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, > } > > > +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" > + > static int > virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, > char **const groups, > void *data) > { > virStorageVolDefPtr vol = NULL; > + bool is_new_vol = false; > unsigned long long offset, size, length; > + const char *regex_unit = "(\\S+)\\((\\S+)\\)"; > + char *regex = NULL; > + regex_t *reg = NULL; > + regmatch_t *vars = NULL; > + char *p = NULL; > + int i, err, nextents, nvars, ret = -1; > > /* See if we're only looking for a specific volume */ > if (data != NULL) { > @@ -88,19 +97,18 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, > return -1; > } > > + is_new_vol = true; > vol->type = VIR_STORAGE_VOL_BLOCK; > > if ((vol->name = strdup(groups[0])) == NULL) { > virReportOOMError(); > - virStorageVolDefFree(vol); > - return -1; > + goto cleanup; > } > > if (VIR_REALLOC_N(pool->volumes.objs, > pool->volumes.count + 1)) { > virReportOOMError(); > - virStorageVolDefFree(vol); > - return -1; > + goto cleanup; > } > pool->volumes.objs[pool->volumes.count++] = vol; > } > @@ -109,8 +117,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, > if (virAsprintf(&vol->target.path, "%s/%s", > pool->def->target.path, vol->name)< 0) { > virReportOOMError(); > - virStorageVolDefFree(vol); > - return -1; > + goto cleanup; > } > } > > @@ -118,8 +125,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, > if (virAsprintf(&vol->backingStore.path, "%s/%s", > pool->def->target.path, groups[1])< 0) { > virReportOOMError(); > - virStorageVolDefFree(vol); > - return -1; > + goto cleanup; > } > > vol->backingStore.format = VIR_STORAGE_POOL_LOGICAL_LVM2; > @@ -128,47 +134,127 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, > if (vol->key == NULL&& > (vol->key = strdup(groups[2])) == NULL) { > virReportOOMError(); > - return -1; > + goto cleanup; > } > > if (virStorageBackendUpdateVolInfo(vol, 1)< 0) > - return -1; > + goto cleanup; > > + nextents = 1; > + if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) { > + if (virStrToLong_i(groups[5], NULL, 10,&nextents)< 0) { > + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("malformed volume extent stripes value")); > + goto cleanup; > + } > + } > > /* Finally fill in extents information */ > if (VIR_REALLOC_N(vol->source.extents, > - vol->source.nextent + 1)< 0) { > + vol->source.nextent + nextents)< 0) { > virReportOOMError(); > - return -1; > + goto cleanup; > } > > - if ((vol->source.extents[vol->source.nextent].path = > - strdup(groups[3])) == NULL) { > + if (virStrToLong_ull(groups[6], NULL, 10,&length)< 0) { > + virStorageReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("malformed volume extent length value")); > + goto cleanup; > + } > + if (virStrToLong_ull(groups[7], NULL, 10,&size)< 0) { > + virStorageReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("malformed volume extent size value")); > + goto cleanup; > + } > + > + /* Now parse the "devices" feild seperately */ > + regex = strdup(regex_unit); > + > + for (i = 1; i< nextents; i++) { > + if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2)< 0) { > + virReportOOMError(); > + goto cleanup; > + } > + /* "," is the seperator of "devices" field */ > + strcat(regex, ","); > + strncat(regex, regex_unit, strlen(regex_unit)); > + } > + > + if (VIR_ALLOC(reg)< 0) { > virReportOOMError(); > - return -1; > + goto cleanup; > } > > - if (virStrToLong_ull(groups[4], NULL, 10,&offset)< 0) { > - virStorageReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("malformed volume extent offset value")); > - return -1; > + /* Each extent has a "path:offset" pair, and vars[0] will > + * be the whole matched string. > + */ > + nvars = (nextents * 2) + 1; > + if (VIR_ALLOC_N(vars, nvars)< 0) { > + virReportOOMError(); > + goto cleanup; > } > - if (virStrToLong_ull(groups[5], NULL, 10,&length)< 0) { > + > + err = regcomp(reg, regex, REG_EXTENDED); > + if (err != 0) { > + char error[100]; > + regerror(err, reg, error, sizeof(error)); > virStorageReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("malformed volume extent length value")); > - return -1; > + _("Failed to compile regex %s"), > + error); > + goto cleanup; > } > - if (virStrToLong_ull(groups[6], NULL, 10,&size)< 0) { > - virStorageReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("malformed volume extent size value")); > - return -1; > + > + if (regexec(reg, groups[3], nvars, vars, 0) != 0) { > + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("malformed volume extent devices value")); > + goto cleanup; > } > > - vol->source.extents[vol->source.nextent].start = offset * size; > - vol->source.extents[vol->source.nextent].end = (offset * size) + length; > - vol->source.nextent++; > + p = groups[3]; > > - return 0; > + /* vars[0] is skipped */ > + for (i = 0; i< nextents; i++) { > + int j, len; > + const char *offset_str = NULL; > + > + j = (i * 2) + 1; > + len = vars[j].rm_eo - vars[j].rm_so; > + p[vars[j].rm_eo] = '\0'; > + > + if ((vol->source.extents[vol->source.nextent].path = > + strndup(p + vars[j].rm_so, len)) == NULL) { > + virReportOOMError(); > + goto cleanup; > + } > + > + len = vars[j + 1].rm_eo - vars[j + 1].rm_so; > + if (!(offset_str = strndup(p + vars[j + 1].rm_so, len))) { > + virReportOOMError(); > + goto cleanup; > + } > + > + if (virStrToLong_ull(offset_str, NULL, 10,&offset)< 0) { > + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("malformed volume extent offset value")); > + 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++; > + } > + > + ret = 0; > + > +cleanup: > + VIR_FREE(regex); > + VIR_FREE(reg); > + VIR_FREE(vars); > + if (is_new_vol&& (ret == -1)) > + virStorageVolDefFree(vol); > + return ret; > } > > static int > @@ -193,15 +279,15 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, > * not a suitable separator (rhbz 470693). > */ > const char *regexes[] = { > - "^\\s*(\\S+),(\\S*),(\\S+),(\\S+)\\((\\S+)\\),(\\S+),([0-9]+),?\\s*$" > + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#?\\s*$" > }; > int vars[] = { > - 7 > + 8 > }; > const char *prog[] = { > - LVS, "--separator", ",", "--noheadings", "--units", "b", > + LVS, "--separator", "#", "--noheadings", "--units", "b", > "--unbuffered", "--nosuffix", "--options", > - "lv_name,origin,uuid,devices,seg_size,vg_extent_size", > + "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size", > pool->def->source.name, NULL > }; > ACK, based on testing this patch with a couple of vol groups, some with and some without stripped volumes Regards, Daniel