On Mon, Mar 09, 2009 at 09:16:45AM +0900, Ryota Ozaki wrote: > Hi, > > storage_conf.c always sets owner/group permissions as 0, even if non-0 values > are specified in XML. Because XPaths in DefParsePerms functions are wrong and > XPath functions using the XPaths fail, as a result the values obtained > by getpid()/getgid() > are used instead. > > This patch fixes this and also unifies duplicated functions, > virStoragePoolDefParsePerms > and virStorageVolDefParsePerms, as a common function virStorageDefParsePerms > to fix the bug prettily. > > One concern I have is the default value of mode permission in > virStorageDefParsePerms > is now 0700 but that in virStorageVolDefParsePerms was 0600. Is this > considerable > change?  Hi Ozaki, the patch looks fine, I like the refactoring idea using relative XPath queries to make the lookup code more reusable. For the default mode, I think the simplest is to add an extra argument 'defaultmode' to virStorageDefParsePerms.

However I think there is a problem with the patch I could not spot: when running the regression tests (make check in a tree checkout) I get 11 failing tests after applying the patch (and none before), I'm afraid somehow the new code crashes in some case but I could not find there (would need to run some of the tests under a debugger).  Would you mind looking at this ?

Daniel -- Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/ daniel(a)veillard.com  | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library  http://libvirt.org/