[libvirt] [PATCH v2 5/5] storage: fs: Only force directory permissions if required

[John Ferlan]

On 05/05/2015 12:44 PM, 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.
> ---
> v2:
>     Fix style issue pointed out by pkrempa
>     Skip chmod if mode == -1 for the fork/nfs case as well
> 
>  src/storage/storage_backend_fs.c | 16 +++++++++++-----
>  src/util/virfile.c               |  4 ++--
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 

A bit of bikeshedding, but adding a local create_flags =
VIR_DIR_CREATE_ALLOW_EXIST and OR'ing in VIR_DIR_CREATE_AS_UID would
enhance readability...


ACK (either way)

John

> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index ed56935..3f42a5b 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);
> @@ -797,18 +799,22 @@ 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_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) {
>          goto error;
>      }
>  
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 63eafdf..5ff4668 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 (chmod(path, mode) < 0) {
> +    if (mode != (mode_t) -1 && chmod(path, mode) < 0) {
>          ret = -errno;
>          virReportSystemError(errno,
>                               _("cannot set mode of '%s' to %04o"),
> @@ -2424,7 +2424,7 @@ virDirCreate(const char *path,
>                               path, (unsigned int) gid);
>          goto childerror;
>      }
> -    if (chmod(path, mode) < 0) {
> +    if (mode != (mode_t) -1 && chmod(path, mode) < 0) {
>          virReportSystemError(errno,
>                               _("cannot set mode of '%s' to %04o"),
>                               path, mode);
>