[libvirt] [PATCH] storage: fix volume perms when it is not specified.

[John Ferlan]

On 1/18/19 11:00 AM, Julio Faracco wrote:
> This commit adds permissions inheritance to volume from main pool when
> it is not explicitly added by command or XML definition. It permissions
> are defined into XML, they should be respected.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=677242
> 
> Signed-off-by: Julio Faracco <jcfaracco at gmail.com>
> ---
>  src/storage/storage_driver.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Although I see the point of the referenced bz - the bz and patch goes
against the documented behavior, see:

https://libvirt.org/formatstorage.html

...
Storage volume XML
...
Target elements
...
permissions
...

"The mode element contains the octal permission set. The mode defaults
to 0600 when not provided. The owner element contains the numeric user
ID. The group element contains the numeric group ID. If owner or group
aren't specified when creating a supported volume, the values are
inherited from the parent directory."

Check out commit 7c2d65dd and fafcc818f for doc and code changes which
are the source of today's logic. If you follow those changes, you'll see
that creation of pool and volume has always had some sort of default;
however, determining when that default was provided or when perhaps user
provided that same value is impossible. So, the usage of -1 for fields
was done in order to help make that determination (for output purposes).

If you did change to use this default, then what would that do to the
existing code using VIR_STORAGE_DEFAULT_VOL_PERM_MODE (O600)? Or how
would code distinguish between something a consumer set and what is/was
set "by default".

BTW: If you took using a storage pool permissions to be the permissions
for the volume, then you'd need to consider that the default storage
pool permissions is handled via VIR_STORAGE_DEFAULT_POOL_PERM_MODE
(O711) which I hope you can agree/understand why you may not want that
as your storage volume value (e.g. executable).

In the long run, defaults are defaults and if someone wants something
other than the default, then they have the capability via input XML to
change those defaults. One thing that could be done is adding mode, gid,
and uid values for input for virsh vol-create-as which creates XML like
would be input during vol-create.

Another possibility for a resolution - maybe add something to a pool's
config or attributes that tells volume creation it's preferable to match
pool permissions rather than take the default when the volume XML
doesn't provide it's own permissions. Maybe that's how you solve that
bug. Maybe there's other opinions out there...

John


> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 4a13e90481..5961d35f26 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1850,6 +1850,16 @@ storageVolCreateXML(virStoragePoolPtr pool,
>          goto cleanup;
>      }
>  
> +    /* Inherit perms and mode from pool when they are not defined. */
> +    if (voldef->target.perms->uid == (uid_t)-1)
> +        voldef->target.perms->uid = def->target.perms.uid;
> +
> +    if (voldef->target.perms->gid == (gid_t)-1)
> +        voldef->target.perms->gid = def->target.perms.gid;
> +
> +    if (voldef->target.perms->mode == (mode_t)-1)
> +        voldef->target.perms->mode = def->target.perms.mode;
> +
>      if (virStorageVolCreateXMLEnsureACL(pool->conn, def, voldef) < 0)
>          goto cleanup;
>  
>