[libvirt] [PATCH] storage: Set the perms if the pool target already exists for fs pools
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(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 169037b..ed0d6ec 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -789,30 +789,32 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, /* 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. */ + uid_t uid; + gid_t gid; - struct stat st; - - if ((stat(pool->def->target.path, &st) < 0) - || (pool->def->target.perms.uid != -1)) { - - uid_t uid = (pool->def->target.perms.uid == -1) - ? getuid() : pool->def->target.perms.uid; - gid_t gid = (pool->def->target.perms.gid == -1) - ? getgid() : pool->def->target.perms.gid; - - if ((err = virDirCreate(pool->def->target.path, - pool->def->target.perms.mode, - uid, gid, - VIR_DIR_CREATE_FORCE_PERMS | - VIR_DIR_CREATE_ALLOW_EXIST | - (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_DIR_CREATE_AS_UID : 0)) < 0)) { - virReportSystemError(-err, _("cannot create path '%s'"), - pool->def->target.path); - goto error; - } + uid = (pool->def->target.perms.uid == -1) + ? getuid() : pool->def->target.perms.uid; + gid = (pool->def->target.perms.gid == -1) + ? getgid() : pool->def->target.perms.gid; + + if ((err = virDirCreate(pool->def->target.path, + pool->def->target.perms.mode, + uid, gid, + VIR_DIR_CREATE_FORCE_PERMS | + VIR_DIR_CREATE_ALLOW_EXIST | + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_DIR_CREATE_AS_UID : 0)) < 0)) { + virReportSystemError(-err, _("cannot create path '%s'"), + pool->def->target.path); + goto error; } + /* Reflect the actual uid and gid to the config. */ + if (pool->def->target.perms.uid == -1) + pool->def->target.perms.uid = uid; + if (pool->def->target.perms.gid == -1) + pool->def->target.perms.gid = gid; + if (flags != 0) { ret = virStorageBackendMakeFileSystem(pool, flags); } else { -- 1.7.7.3
On 06/18/2012 03:57 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.
- uid_t uid = (pool->def->target.perms.uid == -1) - ? getuid() : pool->def->target.perms.uid; - gid_t gid = (pool->def->target.perms.gid == -1) - ? getgid() : pool->def->target.perms.gid; -
+ uid = (pool->def->target.perms.uid == -1)
Pre-existing, but comparison of uid_t against -1 is not portable; you need an explicit cast: pool->def->target.perms-uid == (uid_t) -1
+ /* Reflect the actual uid and gid to the config. */ + if (pool->def->target.perms.uid == -1) + pool->def->target.perms.uid = uid; + if (pool->def->target.perms.gid == -1) + pool->def->target.perms.gid = gid;
And again. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 169037b..bde4528 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -789,30 +789,32 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, /* 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. */ + uid_t uid; + gid_t gid; - struct stat st; - - if ((stat(pool->def->target.path, &st) < 0) - || (pool->def->target.perms.uid != -1)) { - - uid_t uid = (pool->def->target.perms.uid == -1) - ? getuid() : pool->def->target.perms.uid; - gid_t gid = (pool->def->target.perms.gid == -1) - ? getgid() : pool->def->target.perms.gid; - - if ((err = virDirCreate(pool->def->target.path, - pool->def->target.perms.mode, - uid, gid, - VIR_DIR_CREATE_FORCE_PERMS | - VIR_DIR_CREATE_ALLOW_EXIST | - (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_DIR_CREATE_AS_UID : 0)) < 0)) { - virReportSystemError(-err, _("cannot create path '%s'"), - pool->def->target.path); - goto error; - } + 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, + VIR_DIR_CREATE_FORCE_PERMS | + VIR_DIR_CREATE_ALLOW_EXIST | + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_DIR_CREATE_AS_UID : 0)) < 0)) { + virReportSystemError(-err, _("cannot create path '%s'"), + pool->def->target.path); + goto error; } + /* Reflect the actual uid and gid to the config. */ + if (pool->def->target.perms.uid == (uid_t) -1) + pool->def->target.perms.uid = uid; + if (pool->def->target.perms.gid == (gid_t) -1) + pool->def->target.perms.gid = gid; + if (flags != 0) { ret = virStorageBackendMakeFileSystem(pool, flags); } else { -- 1.7.7.3
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2012年06月21日 01:04, Eric Blake wrote:
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.
Thanks, pushed, yeah, agreed, I will do it in a follow up patch. Osier
participants (2)
-
Eric Blake -
Osier Yang