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

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

[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