[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.
> 

Will do

>>          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.
> 

Hmm, I think instead I'll add an additional patch to drop FORCE_PERMS
entirely.. it's used by every virDirCreate caller. We can just key the chmod
off of whether mode != -1

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]