Re: [libvirt] [PATCH 6/6] storage: fs: Only force directory permissions if required

On 04/28/2015 09:36 AM, Peter Krempa wrote: > On Mon, Apr 27, 2015 at 16:48:44 -0400, Cole Robinson wrote: >> Only set directory permissions at pool build time, if: >> >> - User explicitly requested a mode via the XML >> - The directory needs to be created >> - We need to do the crazy NFS root-squash workaround >> >> This allows qemu:///session to call build on an existing directory >> like /tmp. >> --- >> src/storage/storage_backend_fs.c | 16 +++++++++++----- >> src/util/virfile.c | 2 +- >> 2 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c >> index 344ee4c..e11c240 100644 >> --- a/src/storage/storage_backend_fs.c >> +++ b/src/storage/storage_backend_fs.c >> @@ -769,6 +769,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, >> int err, ret = -1; >> char *parent = NULL; >> char *p = NULL; >> + mode_t mode; >> + bool needs_create_as_uid; >> >> virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | >> VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); >> @@ -802,19 +804,23 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, >> } >> } >> >> + needs_create_as_uid = (pool->def->type == VIR_STORAGE_POOL_NETFS); >> + mode = pool->def->target.perms.mode; >> + if (mode == (mode_t) -1 && >> + (needs_create_as_uid || !virFileExists(pool->def->target.path))) >> + mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE; >> + >> /* 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. */ >> if ((err = virDirCreate(pool->def->target.path, >> - (pool->def->target.perms.mode == (mode_t) -1 ? >> - VIR_STORAGE_DEFAULT_POOL_PERM_MODE : >> - pool->def->target.perms.mode), >> + mode, >> pool->def->target.perms.uid, >> pool->def->target.perms.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) { >> + (needs_create_as_uid ? VIR_DIR_CREATE_AS_UID : 0) >> + )) < 0) { > > Closing parentheses on a separate line are ugly. I'd rather see > violating the 80 cols rule rather than damaging readability of the code. > >> goto error; >> } >> >> diff --git a/src/util/virfile.c b/src/util/virfile.c >> index 676e7b4..7e49f39 100644 >> --- a/src/util/virfile.c >> +++ b/src/util/virfile.c >> @@ -2311,7 +2311,7 @@ virDirCreateNoFork(const char *path, >> path, (unsigned int) uid, (unsigned int) gid); >> goto error; >> } >> - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) >> + if (((flags & VIR_DIR_CREATE_FORCE_PERMS) && mode != (mode_t) -1) > > This is a usage error of the function. Using mode == -1 and requesting > it to be set doesn't make much sense. > And to clarify, I'll push patches 1-4 when the new release opens, since they had minor changes. Then I'll post a new series with 5-6 + additional patches