[libvirt] [PATCH 4/4] storage: fix uid_t|gid_t handling on 32 bit Linux
Philipp Hahn
hahn at univention.de
Tue Feb 26 12:06:07 UTC 2013
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 at 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/
-------------- next part --------------
b/tests/storagevolxml2xmlout/vol-sheepdog.xml
-------------- next part --------------
A non-text attachment was scrubbed...
Name: storage.html.in
Type: text/x-patch
Size: 4829 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130226/2aa483c4/attachment-0001.bin>
More information about the libvir-list
mailing list