3:38 a.m.
If permissions (mode, uid, gid) are not specified, a new created vol
will get the permissions like:
mode = 0600
uid = -1
gid = -1
This will be a bit surprised if the user define the pool with a
non-root uid/gid, but the new created vol is still defined as
root/root.
This patch changes the behaviour so that the new created vol will
inherit the permissions of parent pool if permission are not
specified.
---
src/conf/storage_conf.c | 32 ++++++++++++++++++++------------
1 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index e893b2d..18675ad 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -539,6 +539,7 @@ static int
virStorageDefParsePerms(xmlXPathContextPtr ctxt,
virStoragePermsPtr perms,
const char *permxpath,
+ virStoragePermsPtr pool_perms,
int defaultmode) {
char *mode;
long v;
@@ -560,9 +561,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
ctxt->node = node;
mode = virXPathString("string(./mode)", ctxt);
- if (!mode) {
- perms->mode = defaultmode;
- } else {
+
+ if (mode) {
char *end = NULL;
perms->mode = strtol(mode, &end, 8);
if (*end || perms->mode < 0 || perms->mode > 0777) {
@@ -572,28 +572,32 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
goto error;
}
VIR_FREE(mode);
+ } else if (pool_perms) {
+ perms->mode = pool_perms->mode;
+ } else {
+ perms->mode = defaultmode;
}
- if (virXPathNode("./owner", ctxt) == NULL) {
- perms->uid = -1;
- } else {
+ if (virXPathNode("./owner", ctxt)) {
if (virXPathLong("number(./owner)", ctxt, &v) < 0) {
virStorageReportError(VIR_ERR_XML_ERROR,
"%s", _("malformed owner
element"));
goto error;
}
perms->uid = (int)v;
+ } else if (pool_perms) {
+ perms->uid = pool_perms->uid;
}
- if (virXPathNode("./group", ctxt) == NULL) {
- perms->gid = -1;
- } else {
+ if (virXPathNode("./group", ctxt)) {
if (virXPathLong("number(./group)", ctxt, &v) < 0) {
virStorageReportError(VIR_ERR_XML_ERROR,
"%s", _("malformed group
element"));
goto error;
}
perms->gid = (int)v;
+ } else if (pool_perms) {
+ perms->gid = pool_perms->gid;
}
/* NB, we're ignoring missing labels here - they'll simply inherit */
@@ -722,7 +726,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) {
if (virStorageDefParsePerms(ctxt, &ret->target.perms,
- "./target/permissions", 0700) < 0)
+ "./target/permissions", NULL, 0700) < 0)
goto cleanup;
return ret;
@@ -1069,7 +1073,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
}
if (virStorageDefParsePerms(ctxt, &ret->target.perms,
- "./target/permissions", 0600) < 0)
+ "./target/permissions",
+ &pool->target.perms,
+ 0600) < 0)
goto cleanup;
node = virXPathNode("./target/encryption", ctxt);
@@ -1100,7 +1106,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
}
if (virStorageDefParsePerms(ctxt, &ret->backingStore.perms,
- "./backingStore/permissions", 0600) < 0)
+ "./backingStore/permissions",
+ &pool->target.perms,
+ 0600) < 0)
goto cleanup;
return ret;
--
1.7.6
12:05 p.m.
New subject: [libvirt] [PATCH] storage: Inherit permissions of parent pool if they are not specified
On 09/20/2011 04:38 AM, Osier Yang wrote:
If permissions (mode, uid, gid) are not specified, a new created vol
will get the permissions like:
mode = 0600
uid = -1
gid = -1
This will be a bit surprised if the user define the pool with a
non-root uid/gid, but the new created vol is still defined as
root/root.
This patch changes the behaviour so that the new created vol will
inherit the permissions of parent pool if permission are not
specified.
Should this behavior maybe be changed later on when the definition is
used, rather than during parsing? I tend to not like modifying the
incoming data as part of a parse (although I know we're already doing
that in some other places).
(Of course other people may have a different opinion, or there may be a
reason why my suggestion isn't feasible...)
---
src/conf/storage_conf.c | 32 ++++++++++++++++++++------------
1 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index e893b2d..18675ad 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -539,6 +539,7 @@ static int
virStorageDefParsePerms(xmlXPathContextPtr ctxt,
virStoragePermsPtr perms,
const char *permxpath,
+ virStoragePermsPtr pool_perms,
int defaultmode) {
char *mode;
long v;
@@ -560,9 +561,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
ctxt->node = node;
mode = virXPathString("string(./mode)", ctxt);
- if (!mode) {
- perms->mode = defaultmode;
- } else {
+
+ if (mode) {
char *end = NULL;
perms->mode = strtol(mode,&end, 8);
if (*end || perms->mode< 0 || perms->mode> 0777) {
@@ -572,28 +572,32 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
goto error;
}
VIR_FREE(mode);
+ } else if (pool_perms) {
+ perms->mode = pool_perms->mode;
+ } else {
+ perms->mode = defaultmode;
}
- if (virXPathNode("./owner", ctxt) == NULL) {
- perms->uid = -1;
- } else {
+ if (virXPathNode("./owner", ctxt)) {
if (virXPathLong("number(./owner)", ctxt,&v)< 0) {
virStorageReportError(VIR_ERR_XML_ERROR,
"%s", _("malformed owner
element"));
goto error;
}
perms->uid = (int)v;
+ } else if (pool_perms) {
+ perms->uid = pool_perms->uid;
}
- if (virXPathNode("./group", ctxt) == NULL) {
- perms->gid = -1;
- } else {
+ if (virXPathNode("./group", ctxt)) {
if (virXPathLong("number(./group)", ctxt,&v)< 0) {
virStorageReportError(VIR_ERR_XML_ERROR,
"%s", _("malformed group
element"));
goto error;
}
perms->gid = (int)v;
+ } else if (pool_perms) {
+ perms->gid = pool_perms->gid;
}
/* NB, we're ignoring missing labels here - they'll simply inherit */
@@ -722,7 +726,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) {
if (virStorageDefParsePerms(ctxt,&ret->target.perms,
- "./target/permissions", 0700)< 0)
+ "./target/permissions", NULL, 0700)< 0)
goto cleanup;
return ret;
@@ -1069,7 +1073,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
}
if (virStorageDefParsePerms(ctxt,&ret->target.perms,
- "./target/permissions", 0600)< 0)
+ "./target/permissions",
+&pool->target.perms,
+ 0600)< 0)
goto cleanup;
node = virXPathNode("./target/encryption", ctxt);
@@ -1100,7 +1106,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
}
if (virStorageDefParsePerms(ctxt,&ret->backingStore.perms,
- "./backingStore/permissions", 0600)< 0)
+ "./backingStore/permissions",
+&pool->target.perms,
+ 0600)< 0)
goto cleanup;
return ret;
2:27 p.m.
New subject: [libvirt] [PATCH] storage: Inherit permissions of parent pool if they are not specified
On 09/20/2011 11:05 AM, Laine Stump wrote:
On 09/20/2011 04:38 AM, Osier Yang wrote:
> If permissions (mode, uid, gid) are not specified, a new created vol
> will get the permissions like:
>
> mode = 0600
> uid = -1
> gid = -1
>
> This will be a bit surprised if the user define the pool with a
> non-root uid/gid, but the new created vol is still defined as
> root/root.
>
> This patch changes the behaviour so that the new created vol will
> inherit the permissions of parent pool if permission are not
> specified.
Should this behavior maybe be changed later on when the definition is
used, rather than during parsing? I tend to not like modifying the
incoming data as part of a parse (although I know we're already doing
that in some other places).
(Of course other people may have a different opinion, or there may be a
reason why my suggestion isn't feasible...)
I actually like the idea of keeping the user's data intact as long as
possible, so I agree with Laine on refactoring the patch to delay the
inheritance to the point of use when uid is still -1 (rather than
reassigning uid at the time of parse).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:40 p.m.
New subject: [libvirt] [PATCH] storage: Inherit permissions of parent pool if they are not specified
于 2011年09月21日 03:27, Eric Blake 写道:
On 09/20/2011 11:05 AM, Laine Stump wrote:
> On 09/20/2011 04:38 AM, Osier Yang wrote:
>> If permissions (mode, uid, gid) are not specified, a new created vol
>> will get the permissions like:
>>
>> mode = 0600
>> uid = -1
>> gid = -1
>>
>> This will be a bit surprised if the user define the pool with a
>> non-root uid/gid, but the new created vol is still defined as
>> root/root.
>>
>> This patch changes the behaviour so that the new created vol will
>> inherit the permissions of parent pool if permission are not
>> specified.
>
> Should this behavior maybe be changed later on when the definition is
> used, rather than during parsing? I tend to not like modifying the
> incoming data as part of a parse (although I know we're already doing
> that in some other places).
>
> (Of course other people may have a different opinion, or there may be a
> reason why my suggestion isn't feasible...)
I actually like the idea of keeping the user's data intact as long as
possible, so I agree with Laine on refactoring the patch to delay the
inheritance to the point of use when uid is still -1 (rather than
reassigning uid at the time of parse).
Agreed.
So I guess it's time to introduce some flags for virStorageVolumeCreateXML.
Such as:
typedef enum {
VIR_STORAGE_VOL_CREATE_INHERIT_PERMS_GRACEFUL (1 << 0),
VIR_STORAGE_VOL_CREATE_INHERIT_PERMS_FORCE (1 << 1),
} virStorageVolCreateFlags;
If *_INHERIT_PERMS_GRACEFULL is specified, the new created vol will
inherit permissions of parent's pool only when the perms are not specified.
E.g. A vol XML contains perms XML like:
<target>
<permissions>
<mode>0755</mode>
<gid>500</gid>
</permissions>
</target>
Then only *uid* will inherit from parent pool.
If *_INHERIT_PERMS_FORCE is specified, the new created vol will inherit
permissions of parent's pool anyway. This gives a choice for the user
who always want to inherite perms.
Thoughts?
Thanks
Osier
4811
days inactive
4812
days old
3 comments
3 participants
participants (3)
-
Eric Blake -
Laine Stump -
Osier Yang