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@gmail.com> Signed-off-by: Vasiliy Tolstov <v.tolstov@selfip.ru> --- 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); +

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

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. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list