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

[Cole Robinson]

On 1/25/19 4:52 PM, John Ferlan wrote:
> 
> 
> 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."
> 

Actually it looks like these docs are wrong too, in my testing new
volumes do not inherit uid/gid from the parent pool, at least not for a
directory pool. I wrote those docs but I'm not sure if they were ever
correct, maybe it was wishful thinking?

Indeed I just ran v1.2.16 from danpb's virt-ark repo, set up a
qemu:///system dir pool owned by crobinso:crobinso, but by default
volumes are created with root:root, basically the user libvirtd is
running as. I'll send a patch to correct it

> 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.

I agree, at this point changing the defaults could surprise some people
and the current behavior hasn't generated many complaints, so best to
just leave it as is. Apps could override this behavior easily enough if
they wanted, by copying uid/gid out of the parent XML and including it
in the new volume XML.

- Cole