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@xxxxxxxxx> > --- > 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; > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
