Hi Daniel, On Wed, Mar 11, 2009 at 5:27 PM, Daniel Veillard <veillard@xxxxxxxxxx> wrote: > 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. I think that's fine. I will revise that way. > 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 ? umm, I have nothing in mind, but anyway I will investigate that soon. Thank you for reviewing and sorry for not doing tests before submitting. ozaki-r > Daniel > > -- > Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ > daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ > http://veillard.com/ | virtualization library http://libvirt.org/ > -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list