6:06 a.m.
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?
Ask libxml2/libxml/xpath.h:
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
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(a)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