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

[Cole Robinson]

On 04/28/2015 09:54 PM, Cole Robinson wrote:
> 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
> 

Pushed patches 1-4 now.

Thanks,
Cole