[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