[libvirt] [PATCH] storage: Inherit permissions of parent pool if they are not specified

[Laine Stump]

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;