Re: [PATCH v2] storage: Set the perms if the pool target already exists for fs pools

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/19/2012 02:15 AM, Osier Yang wrote:
> The comment says:
> 
> /* Now create the final dir in the path with the uid/gid/mode
>  * requested in the config. If the dir already exists, just set
>  * the perms.
>  */
> 
> However, virDirCreate is only invoked if the target path doesn't
> exist yet (which is opposite with the comment), or the uid from
> the config is not -1 (I don't understand why, think it's just
> another mistake). And the result is the perms of the pool won't
> be changed if one tries to build the pool with different perms
> again.
> 
> Besides these logic error fix, if no uid and gid are specified in
> the config, the practical used uid, gid are reflected.
> ---
>  src/storage/storage_backend_fs.c |   44 +++++++++++++++++++------------------
>  1 files changed, 23 insertions(+), 21 deletions(-)

ACK.  But I wonder if a followup patch should improve things...


> +    uid = (pool->def->target.perms.uid == (uid_t) -1)
> +        ? getuid() : pool->def->target.perms.uid;
> +    gid = (pool->def->target.perms.gid == (gid_t) -1)
> +        ? getgid() : pool->def->target.perms.gid;
> +
> +    if ((err = virDirCreate(pool->def->target.path,
> +                            pool->def->target.perms.mode,
> +                            uid, gid,

Instead of making callers use getuid(), we could move that logic into
virDirCreate(), similarly to how we recently taught virFileOpenAs to
honor incoming -1 arguments in commit 90e4d68.

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org



Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]