Hello, Am Montag 25 Februar 2013, 15:58:50 schrieb Michal Privoznik:
On 22.02.2013 17:55, Philipp Hahn wrote:
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64. ... This one breaks storagevolxml2xmltest: TEST: storagevolxml2xmltest ... ... OK 7) Storage Vol XML-2-XML vol-sheepdog ... Offset 283 Expect [4294967295</owner> <group>4294967295] Actual [-1</owner> <group>-1] ... FAILED
The attached 1st patch would change the test case, but since I don't know anything about sheepdog, I can't say it that is really the correct change. @Sebastian: Is (uid_t)-1 = (u32)-1 special for Sheepdog or was the file just created by a previous virsh-dump, which outputs UINT_MAX instead of -1? There is one more interesting case in storagepoolxml2xmlout/pool-dir.xml, which should currently fail on 32 bit compiles, but does not, because pool- dumpxml only returns the original pool-xml with user/group=-1 (and updates capacities).
However, the first 3 patches are correct. Even though it's freeze, they are pure bugfixes so I've pushed them.
Thanks. On Monday 25 February 2013 19:46:50 Eric Blake wrote:
I like the standardized name INT_MAX better than the invented name ALLONE.
Better, but not correct: UINT_MAX is what is required here, because INT_MAX is the signed one with MSB=0.
+ double d;
Eww - why do we need to use a double?
struct _xmlXPathObject { xmlXPathObjectType type; xmlNodeSetPtr nodesetval; int boolval; double floatval; xmlChar *stringval; void *user; int index; void *user2; int index2; }; The xpath expression "Number(./owner)" must work for integer and floating point values, so a double is returned by "Number()". If you take a look at virXPathLongBase(), you'll see no such "double", because
Ask libxml2/libxml/xpath.h: the return value of "floatval" is immediately cased to a "long". The now used virXPathNumber() returns a "double".
I like the idea of outputting -1 rather than the unsigned all-ones value; which means you need to respin this patch to avoid testsuite failures where we change the test output.
See attached patch.
Also, anywhere that the parser doesn't accept -1, we need to fix that; accepting -1 as the only valid negative value and requiring all other values to be non-negative makes sense.
I had a look at the relax-ng schema: schemas/storagepool.rng accepts the -1, scheams/storagevol.rng does not. On the other hand the "-1" seems not to be documented on <http://libvirt.org/formatstorage.html> (btw: out of date, see 2. patch) The more I think about the -1 problem, the more I'm getting confused: in virStorageBackendFileSystemBuild() there is some code which reads back the actual owner and group, but my /etc/libvirt/storage/default.xml still contains the -1 / 4294967295, which also seems to be returned by "virsh pool-dumpxml default". Personally I would find the following semantics the least confusing: 1. on define, -1 stands for "the current gid / pid" for the future time, when the pool is started. 2. on create/start, -1 would be my gid/pid for qemu:///session and root:libvirt (or whatever) for qemu:///system . 3. on read back of an active pool, the current uid/gid are returned, so never -1. (the target exists, so the current values could be returned; a -1 is not very useful for the user here, IMHO.) 4. on read back of an passive pool, the original uid/gid (including -1) should be returned. (the target might not exist, so libvirt can only show the intended values) I would like to get that clarified before spending more time on a proper v2, since I now have to switch to a different task requiring my immediate attention.
- Some regression test?
Did you run 'make check' on your patch? We already have XML output that is affected by the change in output format, and can write the test so that we prove that multiple ways of formatting input all get canonicalized to the same output.
No, forgot to do that. Thank you for your review and comments. Much appreciated. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH be open. fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/ b/tests/storagevolxml2xmlout/vol-sheepdog.xml