On 11/19/2015 03:44 AM, Vasiliy Tolstov wrote:
> Some storage backends allows to specify per volume redundancy options.
> Sheepdog use x format for specify copies, and x:y format to specify
> data and parity block count.
So you may understand what redundancy is, but I don't believe it's
conveyed well through this commit message. using "x format" or "x:y
format" doesn't help me understand what the 'feature' is, how it's
expected to be used, and why you chose to implement this the way you
did. Also, when I look at the output generated I see a number. I'm not
sure why you chose a string.
Probably should also split this patch up a bit into separable components
such as adding a new field to the rng, which includes the structure
(.h), and code (.c) changes in order to parse, format, and in this case
"free" the data field. The patch would also have *new* tests to validate
the new/optional field. It would also require an adjustment to the
formatstorage.html.in. I know there are examples of previous checkins
that could be used.
After reading more closely there are at least 4 additional patches:
* One to use virStringSplit in virStorageBackendSheepdogParseNodeInfo
* One to show some sort of display output change in parsing nodeinfo
(there's a new field)
* One to use virStringSplit in virStorageBackendSheepdogParseVdiList
* One to add the new redundancy field.
Before posting please be sure to run "make check" and "make
syntax-check" for each patch. My run fails with both
storagevolxml2xmltest and storagevolschematest for make check and a lot
more failures for make syntax-check.
Finally, after going through the entire patch - I still don't seen
how/why redundancy was added. It's not used for anything other than
output. So what's the purpose of adding it? My belief is that it should
not be a character string. Rather it should be two numbers - what they
are called/named is still to be determined since I don't understand
their purpose yet.
As for order. I would make the virStringSplit changes and output
difference for NodeInfo first. Then add the new field.
>
> Signed-off-by: Alexey Tyabin <aleksey.tyabin@xxxxxxxxx>
> Signed-off-by: Vasiliy Tolstov <v.tolstov@xxxxxxxxx>
> ---
> docs/schemas/storagevol.rng | 3 +
> src/conf/storage_conf.c | 2 +
> src/storage/storage_backend_sheepdog.c | 143 +++++++++++++++++-----------
> src/util/virstoragefile.c | 4 +-
> src/util/virstoragefile.h | 2 +
> tests/storagebackendsheepdogtest.c | 104 ++++++++++----------
> tests/storagevolxml2xmlin/vol-sheepdog.xml | 1 +
> tests/storagevolxml2xmlout/vol-sheepdog.xml | 1 +
> 8 files changed, 153 insertions(+), 107 deletions(-)
>
> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
> index 7450547..068993f 100644
> --- a/docs/schemas/storagevol.rng
> +++ b/docs/schemas/storagevol.rng
> @@ -55,6 +55,9 @@
> <element name='allocation'>
> <ref name='scaledInteger'/>
> </element>
> + <element name='redundancy'>
> + <ref name='string'/>
> + </element>
Kind of a broad classification. There are examples that would show if
you're expecting a certain format that rather than be so broad, you
could expect a format in a certain manner (look at the IP addresses
perhaps for an example...)
Also what does this have to do with 'sizing'? This should have changed
the <define name='vol'> to have the following below "ref name='sizing'":
<optional>
<ref name='redundancy'/>
</optional>
and
<define name='redundancy'>
...
</define>
I use the ... because I'm not really clear whether using a string is the
correct final format. It would seem it should be two numbers. How those
get represented is still to be determined. At least 1 would be required
and the second would be optional.
> </optional>
> </interleave>
> </define>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 9b8abea..d37c93a 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1345,6 +1345,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
> ret->target.allocation = ret->target.capacity;
> }
>
> + ret->target.redundancy = virXPathString("string(./redundancy)", ctxt);
> +
If this is really "x" or "x:y" as in two numbers, why not read in the
string, then if present use virStringSplit with a ":" as the separator.
That returns an array of strings either as list[0] = "x", list[1] = NULL
or list[0] = "x", list[1] = "y", list[2] = NULL.
>From there you can convert the strings into numbers. If of course the
returned list is bogus, then you can/should have a configuration or
format error here as well. Just carrying this around for some later
future use is well, confusing at this point.
I have a "suspicion" that a "0" redundancy is meaningless, so if you
stored as a pair of integers it may be less confusing and easier to
manipulate and use.
Where's the formatting for the redunancy output, e.g. no change in
virStorageVolDefFormat.
> ret->target.path = virXPathString("string(./target/path)", ctxt);
> if (options->formatFromString) {
> char *format = virXPathString("string(./target/format/@type)", ctxt);
Up to this point - everything's been a patch 1 change. The following
would be a patch 2 type change (e.g. the functional use of the data).
Of course having now read the changes, I believe there's 4 changes going
on #1 is to start using virStringSplit instead of open code parsing in
virStorageBackendSheepdogParseNodeInfo. Then #2 is some sort of format
output change... Then #3 is using virStringSplit in
virStorageBackendSheepdogParseVdiList. Finally #4 is however this
redundancy code is added.
> diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
> index 1200813..d8356a1 100644
> --- a/src/storage/storage_backend_sheepdog.c
> +++ b/src/storage/storage_backend_sheepdog.c
> @@ -51,43 +51,52 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool,
> * node id/total, size, used, use%, [total vdi size]
^^^^^^^^^^^^
What's the new field? That's a format output change that should be
described separately.
Also, note - Existing; however, perhaps it should be "node id/Total"...
> *
> * example output:
> - * 0 15245667872 117571104 0%
> - * Total 15245667872 117571104 0% 20972341
> + * 0 425814278144 4871131136 420943147008 1%
> + * Total 2671562256384 32160083968 2639402172416 1% 75161927680
> */
It's not clear to me what caused the "extra" field. Since you co-mingled
functional change (e.g. adding "-c") with adjusting code to use
virStringSplit - it's not clear why there's an extra field of data. The
"size, used, use%" seems to have a new element, but why?
If the new field is a result of some output change from:
virCommandNewArgList(SHEEPDOGCLI, "node", "info", "-r", NULL);
then how do you account for environments that don't have the new field?
It seems there should be some new argument added. Or maybe the output
needs to "ensure" all that we're printing out is the first 3 fields.
Some more details would probably help...
Luckily it seems that the first two numbers are all that matter, but it
does point out libvirt has a dependency on the output format order. Is
there a way to just print those first 3 fields by some argument to the
sheepdogcli?
> -
> - const char *p, *next;
> + char **lines = NULL;
> + char **cells = NULL;
> + size_t i;
> + int ret = -1;
>
> pool->allocation = pool->capacity = pool->available = 0;
>
> - p = output;
> - do {
> - char *end;
> + lines = virStringSplit(output, "\n", 0);
> + if (lines == NULL)
> + goto cleanup;
>
> - if ((next = strchr(p, '\n')))
> - ++next;
> - else
> - break;
I assume now that this is why your test changed later on to have a
success on "Total 1 2"?
That's OK and proper, but would be something more easily called out in a
single patch change to exhibit the difference when using virStringSplit
instead of the ad hoc parsing.
> + for (i = 0; lines[i]; i++) {
> + char *line = lines[i];
> + if (line == NULL)
> + goto cleanup;
>
> - if (!STRPREFIX(p, "Total "))
> + if (!STRPREFIX(line, "Total "))
> continue;
In particular, it seems we *only* care about line(s)? that start with
Total - is that a fair assessment? Too bad there's no way to make only
the Total show up with some option to the sheepdogcli.
>
> - p = p + 6;
> + virStringStripControlChars(line);
> + virTrimSpaces(line, NULL);
Why are these being used?
> + if ((cells = virStringSplit(line, " ", 0)) == NULL)
> + continue;
If there can only be one "Total" line, then shouldn't this be an error?
>
> - if (virStrToLong_ull(p, &end, 10, &pool->capacity) < 0)
> - break;
> + if (virStringListLength(cells) < 3) {
> + goto cleanup;
> + }
make syntax-check would have told you a single line if statement doesn't
need { }; however, I think this should actually be changed to show an
error prior to goto cleanup - e.g.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
"invalid sheepdog cli output - missing fields");
(or something like that)
>
> - if ((p = end + 1) > next)
> - break;
> + if (virStrToLong_ull(cells[1], NULL, 10, &pool->capacity) < 0)
> + goto cleanup;
>
> - if (virStrToLong_ull(p, &end, 10, &pool->allocation) < 0)
> - break;
> + if (virStrToLong_ull(cells[2], NULL, 10, &pool->allocation) < 0)
> + goto cleanup;
>
> pool->available = pool->capacity - pool->allocation;
> - return 0;
> -
> - } while ((p = next));
> + ret = 0;
> + break;
So this says to me there can only be one "Total" line
> + }
But if I get here and ret == -1, that means "Total" wasn't found which
is something we probably also need to output as an error, e.g.
if (ret == -1)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
"invalid sheepdog cli output - missing Total");
(or something like that)
>
> - return -1;
> + cleanup:
> + virStringFreeList(lines);
> + virStringFreeList(cells);
> + return ret;
> }
>
> void
> @@ -275,6 +284,10 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn ATTRIBUTE_UNUSED,
>
> cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "create", vol->name, NULL);
> virCommandAddArgFormat(cmd, "%llu", vol->target.capacity);
> +
> + if(NULL != vol->target.redundancy)
Wrong order/comparison - libvirt coding standards use:
"if (vol->target.redundancy != NULL)"
however, it's just as good to use :
if (!vol->target.redundancy)"
although I'm still not convinced redundancy should be a string... Could
just as easily be a number where a 0 (zero) would have the same meaning
of not needing the "-c"...
> + virCommandAddArgList(cmd, "-c", vol->target.redundancy, NULL);
> +
> virStorageBackendSheepdogAddHostArg(cmd, pool);
> if (virCommandRun(cmd, NULL) < 0)
> goto cleanup;
> @@ -291,60 +304,78 @@ virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol,
> char *output)
> {
> /* fields:
> - * current/clone/snapshot, name, id, size, used, shared, creation time, vdi id, [tag]
> + * current/clone/snapshot, name, id, size, used, shared, creation time, vdi id, redundancy, [tag], size shift
> *
Could you elaborate a bit further on the format expectations of
"current", "clone", and "snapshot". That is a line that starts with "s"
is a ?what?... A line that starts with "=" is a ?what?. What other
oddities may be found? It seems as though from my read of the code that
it's possible that a line just has an "=" on it, then that's not good.
NB: Would knowing the "size shift" ever be important? Why does adding a
"-c" cause it to be output as well?
>From the output below, it also seems that if "-c" is supplied, then
"[tag]" is perhaps no longer optional since I see the tt, zz, xx in the
output. Is that true? Although later in the test code it's not there.
> * example output:
> - * s test 1 10 0 0 1336556634 7c2b25
> - * s test 2 10 0 0 1336557203 7c2b26
> - * = test 3 10 0 0 1336557216 7c2b27
> + * s test 1 10 0 0 1336556634 7c2b25 1 tt 22
> + * s test 2 10 0 0 1336557203 7c2b26 2 zz 22
> + * = 39865 0 21474836480 247463936 1337982976 1447516646 47d187 2 22
^^^^^
? What is this format? A number instead of name? Not sure I
understand. Is that a result of using the new "-c"? Would it show up
with the "-c"?
The problem I think this code is going to face is how to determine
whether "-c" was provided or not, but we'll see.
Also recalling from earlier - redundancy can either be "x" or "x:y",
right? So how is does that look?
> + * = test 3 10 0 0 1336557216 7c2b27 3 xx 22
> */
> -
> - int id;
> - const char *p, *next;
> + char **lines = NULL;
> + char **cells = NULL;
> + size_t i;
> + int ret = -1;
>
> vol->target.allocation = vol->target.capacity = 0;
> + vol->target.redundancy = NULL;
I think this should become two numbers rather than char string...
>
> - p = output;
> - do {
> - char *end;
> + lines = virStringSplit(output, "\n", 0);
> + if (lines == NULL)
> + goto cleanup;
>
> - if ((next = strchr(p, '\n')))
> - ++next;
> + for (i = 0; lines[i]; i++) {
> + char *line = lines[i];
> + if (line == NULL)
> + break;
>
> - /* ignore snapshots */
restore the comment
> - if (*p != '=')
> + if (!STRPREFIX(line, "= "))
Why not just change the "*p" to "*line"?
> continue;
>
> - /* skip space */
> - if (p + 2 < next)
> - p += 2;
> + /* skip = and space */
> + if (*(line + 2) != '\0')
> + line += 2;
> else
> - return -1;
> + continue;
So old code says - if the line only has "=" on it, then we're returning
an error. The new code says, ignore it. Doesn't seem right. BTW: The
test still fails because lines in the test that have just "=" or "=
test" fail off the end of the while loop.
So does this mean some lines could have nothing after "="? Oddball
case? If it's unexpected, then shouldn't we error out with some sort of
message (to avoid the less than helpful generic there was some unknown
error message given to the user).
>
> /* skip name */
> - while (*p != '\0' && *p != ' ') {
> - if (*p == '\\')
> - ++p;
> - ++p;
> + while (*line != '\0' && *line != ' ') {
> + if (*line == '\\')
> + ++line;
> + ++line;
> }
>
> - if (virStrToLong_i(p, &end, 10, &id) < 0)
> - return -1;
> + /* skip space */
> + if (*(line + 1) != '\0')
> + line += 1;
> + else
> + continue;
>
> - p = end + 1;
Code up through here is really confusing. I don't see the purpose.
Once you have a line that starts with "=", call virStringSplit()
Then you can tell how many "cells" are returned. If it's <= to some
specific value then generate an error message and goto cleanup
> + virStringStripControlChars(line);
> + virTrimSpaces(line, NULL);
Still not sure why these two exist...
> + if ((cells = virStringSplit(line, " ", 0)) == NULL)
> + continue;
If as I point out below there can only be one line with "=", then
shouldn't this be an error?
>
> - if (virStrToLong_ull(p, &end, 10, &vol->target.capacity) < 0)
> - return -1;
> + if (virStringListLength(cells) < 5)
> + continue;
As my coverity checker points out - if you're going to "continue", then
you need a virStringFreeList(cells).
Also if there can only be one line with "=", this doesn't seem right?
>
> - p = end + 1;
> + if ((ret = virStrToLong_ull(cells[1], NULL, 10, &vol->target.capacity)) < 0)
> + goto cleanup;
>
> - if (virStrToLong_ull(p, &end, 10, &vol->target.allocation) < 0)
> - return -1;
> + if ((ret = virStrToLong_ull(cells[2], NULL, 10, &vol->target.allocation)) < 0)
> + goto cleanup;
>
> - return 0;
> - } while ((p = next));
> + if ((ret = VIR_STRDUP(vol->target.redundancy, cells[6])) < 0)
> + goto cleanup;
>
> - return -1;
> + ret = 0;
> + break;
Hmm.. so there can only be one line with "="? Not what it appears in
the sample output above. That's something that needs to be documented
above...
I think it would be a whole lot cleaner with the following algorithm
if (!(lines = virStringSplit(output, "\n", 0)))
goto cleanup
for (i = 0; lines[i], i++) {
if (!(line = lines[i]))
break;
/* Only parse ??? lines */ [eg. whatever lines starting with = are]
if (*line != '=')
continue;
/* No need to report error - virStringSplit should do that for us */
if (!(cells = virStringSplit(line, " ", 0)))
goto cleanup
if ((cellcount = virStringListLength(cells)) < 8) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
"invalid vdi output - no enough data");
goto cleanup;
}
capacity is then the 3rd field of line
allocation is then the 4th field of line
if (cellcount >= 8)
redundancy is the 8th field of line
ret = 0;
break;
}
if (ret == -1)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
"invalid vdi output - no ??? line found");
where ??? is the name/style of the line you're looking for... I assume
sort sort of summary line, but whatever the lines starting with "="
equate to is what would be used.
Although I'm still curious about the single = reference, this to me is
at least shorter. There's no need to skip '=' and 'name', just use the
power of virStringSplit and access the fields you want directly.
I also think that 8th field needs to be parsed into number(s) rather
than kept as a string. What's not clear (yet) is whether the field is
only a single number or whether it could be "x:y". If it can be both,
then both need to be tested later on...
> + }
> +
> + cleanup:
> + virStringFreeList(lines);
> + virStringFreeList(cells);
> + return ret;
> }
>
> static int
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 2aa1d90..9cdc90d 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1846,7 +1846,8 @@ virStorageSourceCopy(const virStorageSource *src,
> VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 ||
> VIR_STRDUP(ret->snapshot, src->snapshot) < 0 ||
> VIR_STRDUP(ret->configFile, src->configFile) < 0 ||
> - VIR_STRDUP(ret->compat, src->compat) < 0)
> + VIR_STRDUP(ret->compat, src->compat) < 0 ||
> + VIR_STRDUP(ret->redundancy, src->redundancy) < 0)
Still not convinced that carrying a string around is the right way...
> goto error;
>
> if (src->nhosts) {
> @@ -2040,6 +2041,7 @@ virStorageSourceClear(virStorageSourcePtr def)
> VIR_FREE(def->volume);
> VIR_FREE(def->snapshot);
> VIR_FREE(def->configFile);
> + VIR_FREE(def->redundancy);
This would change too if string goes away.
> virStorageSourcePoolDefFree(def->srcpool);
> VIR_FREE(def->driverName);
> virBitmapFree(def->features);
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index b98fe25..c37cfc2 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -282,6 +282,8 @@ struct _virStorageSource {
> /* Name of the child backing store recorded in metadata of the
> * current file. */
> char *backingStoreRaw;
Add extra line here to at least separate...
> + /* redundancy level, may be used by sheepdog or ceph */
> + char *redundancy;
So rather than a string, perhaps this should be a numerical
representation...
> };
>
>
I think the following test perhaps gets adjust 2 or 3 times depending on
whether there's any differences with using virStringSplit for vdi parsing.
> diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c
> index 2b0f4db..d110bde 100644
> --- a/tests/storagebackendsheepdogtest.c
> +++ b/tests/storagebackendsheepdogtest.c
> @@ -42,15 +42,23 @@ typedef struct {
> int expected_return;
> uint64_t expected_capacity;
> uint64_t expected_allocation;
> -} collie_test;
> +} collie_test1;
> +
> +typedef struct {
> + const char *output;
> + int expected_return;
> + uint64_t expected_capacity;
> + uint64_t expected_allocation;
> + const char *expected_redundancy;
I think this should be a number or pair of numbers, where "0" would mean
no redundancy "x" and/or "x:y"
> +} collie_test2;
>
> struct testNodeInfoParserData {
> - collie_test data;
> + collie_test1 data;
Seems this is associated with nodeinfo changes
> const char *poolxml;
> };
>
> struct testVDIListParserData {
> - collie_test data;
> + collie_test2 data;
and this with vdi changes
> const char *poolxml;
> const char *volxml;
> };
> @@ -60,7 +68,7 @@ static int
> test_node_info_parser(const void *opaque)
> {
> const struct testNodeInfoParserData *data = opaque;
> - collie_test test = data->data;
> + collie_test1 test = data->data;
> int ret = -1;
> char *output = NULL;
> virStoragePoolDefPtr pool = NULL;
> @@ -75,11 +83,6 @@ test_node_info_parser(const void *opaque)
> test.expected_return)
> goto cleanup;
>
> - if (test.expected_return) {
> - ret = 0;
> - goto cleanup;
> - }
> -
Why was this removed? Is this fixing a separate bug in the test?
Hmmm. I think I see - maybe it would be clearer:
if (test.expected_return == -1) {
ret = 0;
goto cleanup;
}
Once there's a failure, the rest of the data validity is not necessarily
guaranteed and there's no reason to check it.
Not sure it needs to change - perhaps makes a couple changes later in
the test table unnecessary...
> if (pool->capacity == test.expected_capacity &&
> pool->allocation == test.expected_allocation)
> ret = 0;
> @@ -94,7 +97,7 @@ static int
> test_vdi_list_parser(const void *opaque)
> {
> const struct testVDIListParserData *data = opaque;
> - collie_test test = data->data;
> + collie_test2 test = data->data;
> int ret = -1;
> char *output = NULL;
> virStoragePoolDefPtr pool = NULL;
> @@ -113,14 +116,15 @@ test_vdi_list_parser(const void *opaque)
> test.expected_return)
> goto cleanup;
>
> - if (test.expected_return) {
> - ret = 0;
> - goto cleanup;
> - }
Similar to above regarding removing this change...
>
> if (vol->target.capacity == test.expected_capacity &&
> - vol->target.allocation == test.expected_allocation)
> - ret = 0;
> + vol->target.allocation == test.expected_allocation) {
> + if (test.expected_redundancy != NULL && vol->target.redundancy != NULL &&
> + !strcmp(vol->target.redundancy, test.expected_redundancy))
> + ret = 0;
Don't think these should be strings... Furthermore if they were the
libvirt STREQ_NULLABLE could have been used.
Having redundancy as a number just makes this so much easier... it's
either 0, or some specific value. You'd still have two checks one for
the single redundancy style and one for the "x:y" style
> + if (test.expected_redundancy == NULL && vol->target.redundancy == NULL)
> + ret = 0;
> + }
>
> cleanup:
> VIR_FREE(output);
> @@ -137,45 +141,47 @@ mymain(void)
> char *poolxml = NULL;
> char *volxml = NULL;
>
In order to validate that the virStringSplit didn't change the results I
wouldn't expect this table to change at all - for the most part...
> - collie_test node_info_tests[] = {
> + collie_test1 node_info_tests[] = {
> {"", -1, 0, 0},
> - {"Total 15245667872 117571104 0% 20972341\n", 0, 15245667872, 117571104},
Keeping this line I think would be important. It should not just be
removed. It should still be valid output especially since we only care
about fields [1] and [2].
> + {"Total 2671562256384 32160083968 2639402172416 1% 75161927680\n", 0, 2671562256384, 32160083968},
> + {"Total 15245667872 117571104 20972341 0%\n", 0, 15245667872, 117571104},
Adding these two appears to be fine - since it seems for some reason the
output has changed to add that new field. I think that's a separate patch.
> {"To", -1, 0, 0},
> {"asdf\nasdf", -1, 0, 0},
> {"Total ", -1, 0, 0},
> {"Total 1", -1, 0, 0},
> {"Total 1\n", -1, 0, 0},
> {"Total 1 ", -1, 0, 0},
> - {"Total 1 2", -1, 0, 0},
> - {"Total 1 2 ", -1, 0, 0},
> + {"Total 1 2 ", 0, 1, 2},
I guess I can "see" why this is now successful, although I think keeping
both examples would be better. This perhaps is the only change to the
table in the conversion to use virStringSplit
> {"Total 1 2\n", 0, 1, 2},
> {"Total 1 2 \n", 0, 1, 2},
> + {"Total 1 2 \n", 0, 1, 2},
How is this new line different than the previous?
> {"Total a 2 \n", -1, 0, 0},
> - {"Total 1 b \n", -1, 0, 0},
> + {"Total 1 b \n", -1, 1, 0},
OK - yep I see this one - although the -1 return value would seemingly
supersede the need to ever check that the "1"'s match.
> {"Total a b \n", -1, 0, 0},
> {"stuff\nTotal 1 2 \n", 0, 1, 2},
> - {"0 1 2 3\nTotal 1 2 \n", 0, 1, 2},
> + {"0 1 2\nTotal 1 2 \n", 0, 1, 2},
Why change this? You've removed the "3", correct? I don't see that
being an error since we ignore the line any because it doesn't start
with "Total".
> {NULL, 0, 0, 0}
> };
So here's my thoughts - you generate a patch to just change
virStorageBackendSheepdogParseNodeInfo to use virStringSplit, then only
this table gets modified in the same patch in order to validate the
changes.
The addition of those new lines with that extra output field is
something that needs to be explained and perhaps should be its own patch
*prior* to these changes. That is, can the current code without any
patches handle that.
>
The vdi changes will also become their own separate patch - these seem
to be a bit more tricky. Furthermore, there's two patches for this case.
One that just handles the differences by using virStringSplit and the
second that adds the new field (I don't think NULL is right - it should
be a number...
I'm still baffled over why the lines:
virStringStripControlChars(line);
virTrimSpaces(line, NULL);
were added... Should we be testing for control characters here? Is that
a different bug/issue? Should it be its own patch?
> - collie_test vdi_list_tests[] = {
> - {"", -1, 0, 0},
> - {"= test 3 10 20 0 1336557216 7c2b27\n", 0, 10, 20},
> - {"= test\\ with\\ spaces 3 10 20 0 1336557216 7c2b27\n", 0, 10, 20},
> - {"= backslashattheend\\\\ 3 10 20 0 1336557216 7c2b27\n", 0, 10, 20},
> - {"s test 1 10 20 0 1336556634 7c2b25\n= test 3 50 60 0 1336557216 7c2b27\n", 0, 50, 60},
> - {"=", -1, 0, 0},
> - {"= test", -1, 0, 0},
> - {"= test ", -1, 0, 0},
> - {"= test 1", -1, 0, 0},
> - {"= test 1 ", -1, 0, 0},
> - {"= test 1 2", -1, 0, 0},
> - {"= test 1 2 ", -1, 0, 0},
> - {"= test 1 2 3", -1, 0, 0},
> - {NULL, 0, 0, 0}
> + collie_test2 vdi_list_tests[] = {
> + {"", -1, 0, 0,NULL},
> + {"= test 3 10 20 1 1336557216 7c2b27 1 22\n", 0, 10, 20, "1"},
> + {"= test\\ with\\ spaces 3 10 20 0 1336557216 7c2b27 3:4 22\n", 0, 10, 20, "3:4"},
> + {"= backslashattheend\\\\ 3 10 20 0 1336557216 7c2b27 1 22\n", 0, 10, 20, "1"},
> + {"s test 1 10 20 0 1336556634 7c2b25 2\n= test 3 50 60 0 1336557216 7c2b27 2:3 22\n", 0, 50, 60, "2:3"},
> + {"=", -1, 0, 0, NULL},
> + {"= test", -1, 0, 0, NULL},
> + {"= test ", -1, 0, 0,NULL},
> + {"= test 1", -1, 0, 0,NULL},
> + {"= test 1 ", -1, 0, 0,NULL},
> + {"= test 1 2", -1, 0, 0,NULL},
> + {"= test 1 2 ", -1, 0, 0,NULL},
> + {"= test 1 2 3", -1, 0, 0,NULL},
> + {NULL, 0, 0, 0,NULL}
If you hand run make syntax-check, you would have found that it gets
upset with "0,NULL" - it should have been "0, NULL" (extra space).
I personally believe NULL should be replaced by one or two numbers...
Also don't change an existing line to add the new redundancy field - add
a new line that merely is a copy of the previous one, but with a
redundancy value.
I also assume that perhaps "a" or "a:b" would be invalid output/input
for redundancy - that needs to be tested/checked (hence another reason I
feel these should be numbers and not a string that we do nothing with).
> };
>
> - collie_test *test = node_info_tests;
> + collie_test1 *test1 = node_info_tests;
> + collie_test2 *test2 = vdi_list_tests;
>
> if (virAsprintf(&poolxml, "%s/storagepoolxml2xmlin/pool-sheepdog.xml",
> abs_srcdir) < 0)
> @@ -185,10 +191,10 @@ mymain(void)
> abs_srcdir) < 0)
> goto cleanup;
>
> -#define DO_TEST_NODE(collie) \
> +#define DO_TEST_NODE(collie1) \
> do { \
> struct testNodeInfoParserData data = { \
> - .data = collie, \
> + .data = collie1, \
> .poolxml = poolxml, \
> }; \
> if (virtTestRun("node_info_parser", test_node_info_parser, \
> @@ -196,16 +202,16 @@ mymain(void)
> ret = -1; \
> } while (0)
The "\" at the end should line up all the way down...
>
> - while (test->output != NULL) {
> - DO_TEST_NODE(*test);
> - ++test;
> + while (test1->output != NULL) {
> + DO_TEST_NODE(*test1);
> + ++test1;
> }
>
>
> -#define DO_TEST_VDI(collie) \
> +#define DO_TEST_VDI(collie2) \
> do { \
> struct testVDIListParserData data = { \
> - .data = collie, \
> + .data = collie2, \
> .poolxml = poolxml, \
> .volxml = volxml, \
> }; \
> @@ -214,11 +220,9 @@ mymain(void)
> ret = -1; \
> } while (0)
Similar comment regarding the ending "\"
>
> - test = vdi_list_tests;
> -
> - while (test->output != NULL) {
> - DO_TEST_VDI(*test);
> - ++test;
> + while (test2->output != NULL) {
> + DO_TEST_VDI(*test2);
> + ++test2;
> }
>
> cleanup:
The following would be associated with the rng changes; however, rather
than altering an existing test, a new test should have been created
"vol-sheepdog-redundancy.xml" which would utilize the new field. That
would mean modifying storagevolxml2xmltest.c too to add the new test.
Leaving the existing test without the new, optional field to ensure that
by not providing the field something isn't broken.
Furthermore, if there are multiple format expectations on the field (eg,
your earlier comment about "x" and "x:y"), then both of those need a
separate parsing test. That is one "new" test using "x" format and one
new test using "x:y" format.
Curious to see what v4 brings...
John
> diff --git a/tests/storagevolxml2xmlin/vol-sheepdog.xml b/tests/storagevolxml2xmlin/vol-sheepdog.xml
> index d6e920b..f88e6db 100644
> --- a/tests/storagevolxml2xmlin/vol-sheepdog.xml
> +++ b/tests/storagevolxml2xmlin/vol-sheepdog.xml
> @@ -4,6 +4,7 @@
> </source>
> <capacity unit='bytes'>1024</capacity>
> <allocation unit='bytes'>0</allocation>
> + <redundancy unit='string'>3</redundancy>
> <target>
> <path>sheepdog:test2</path>
> </target>
> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml
> index e1d6a9e..207f5fb 100644
> --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml
> +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml
> @@ -4,6 +4,7 @@
> </source>
> <capacity unit='bytes'>1024</capacity>
> <allocation unit='bytes'>0</allocation>
> + <redundancy unit='string'>3</redundancy>
> <target>
> <path>sheepdog:test2</path>
> <format type='unknown'/>
> --
> 2.5.0
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list