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

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.

> && (chmod(path, mode) < 0)) { > ret = -errno; > virReportSystemError(errno, ACK. Peter