On 11/19/2015 03:44 AM, Vasiliy Tolstov wrote: 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. 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. 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. 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. 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. ^^^^^^^^^^^^ 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"... 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? 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. 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. Why are these being used? If there can only be one "Total" line, then shouldn't this be an error? 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) 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) 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"... 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? "[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. ^^^^^ ? 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? I think this should become two numbers rather than char string... restore the comment Why not just change the "*p" to "*line"? 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). 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 Still not sure why these two exist... If as I point out below there can only be one line with "=", then shouldn't this be an error? 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? 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... This would change too if string goes away. Add extra line here to at least separate... 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. I think this should be a number or pair of numbers, where "0" would mean no redundancy "x" and/or "x:y" Seems this is associated with nodeinfo changes and this with vdi changes 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... Similar to above regarding removing this change... 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... 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]. 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. 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 How is this new line different than the previous? OK - yep I see this one - although the -1 return value would seemingly supersede the need to ever check that the "1"'s match. 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". 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? 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list