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

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.