[libvirt] [PATCH] storage: fix disregarding specified owner/group permissions
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? Thanks, ozaki-r
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Hi Daniel, On Wed, Mar 11, 2009 at 5:27 PM, Daniel Veillard <veillard@redhat.com> 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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel Veillard -
Ryota Ozaki