11:50 a.m.
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