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

2015-05-04 Thread 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-04-28 Thread Cole Robinson 
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

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-04-28 Thread Cole Robinson 
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
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-04-28 Thread Peter Krempa 
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


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-04-27 Thread Cole Robinson 
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) {
 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)
 && (chmod(path, mode) < 0)) {
 ret = -errno;
 virReportSystemError(errno,
-- 
2.3.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list