Re: [libvirt] [PATCH] storage: fix volume perms when it is not specified.
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(a)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