[libvirt] [PATCH v2 0/5] storage: fs: tweak dir perms handling on build
Consider the following issue - Using virt-manager with qemu:///session - User adds a storage pool pointing at /tmp. No explicit permissions are requested in the XML - virt-manager calls PoolDefine, then PoolBuild - libvirt tries to unconditionally chmod 755 /tmp. This fails because my user doesn't own root. Pool build fails, virt-manager reports failure Yes there's a couple ways we could avoid this specific case in virt-manager, but I think it makes more sense to have pool.build on a directory be a no-op in this case. The following patches address this. I already pushed some bits of v1 of the series. Now the patches are: - Patch 1 (new) fixes a bug in the previous patches I pushed - Patch 2 (new) changes storage XML to not output -1 for owner/group - Patch 3 changes storage XML to not hardcode a <mode> value if the user didn't specify one in. - Patch 4 (new) drops a redundant flag to virDirCreate - Patch 5 makes pool.build skip dir chmod unless the user explicitly requested <mode> via the XML. However if a mode is required for mkdir, continue to use the previous default. v2: Add patches 1, 3, 4, adjust other patches to match Style fix in patch 5 Cole Robinson (5): virfile: virDirCreate: Fix ALLOW_EXIST conditional storage: conf: Don't output owner/group -1 storage: conf: Don't set any default <mode> in the XML virfile: virDirCreate: Drop redundand FORCE_PERMS flag storage: fs: Only force directory permissions if required docs/schemas/storagecommon.rng | 36 ++++++++------ src/conf/storage_conf.c | 57 +++++++++++----------- src/storage/storage_backend.c | 20 ++++++-- src/storage/storage_backend.h | 3 ++ src/storage/storage_backend_fs.c | 21 +++++--- src/storage/storage_backend_logical.c | 4 +- src/util/virfile.c | 8 ++- src/util/virfile.h | 3 +- tests/storagepoolxml2xmlout/pool-dir-naming.xml | 2 - tests/storagepoolxml2xmlout/pool-dir.xml | 2 - tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 3 -- tests/storagevolxml2xmlin/vol-file.xml | 4 +- .../vol-gluster-dir-neg-uid.xml | 2 - tests/storagevolxml2xmlout/vol-gluster-dir.xml | 3 -- tests/storagevolxml2xmlout/vol-sheepdog.xml | 3 -- 15 files changed, 91 insertions(+), 80 deletions(-) -- 2.4.0
I screwed this up in the previous (post 1.2.16) commits --- src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 8a06813..6e9ecbe 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2289,7 +2289,7 @@ virDirCreateNoFork(const char *path, int ret = 0; struct stat st; - if (!((flags & VIR_DIR_CREATE_ALLOW_EXIST) && !virFileExists(path))) { + if (!((flags & VIR_DIR_CREATE_ALLOW_EXIST) && virFileExists(path))) { if (mkdir(path, mode) < 0) { ret = -errno; virReportSystemError(errno, _("failed to create directory '%s'"), -- 2.4.0
On 05/05/2015 12:43 PM, Cole Robinson wrote:
I screwed this up in the previous (post 1.2.16) commits --- src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK -- although I'll admit the construct is painful to read... John
On 05/08/2015 09:59 AM, John Ferlan wrote:
On 05/05/2015 12:43 PM, Cole Robinson wrote:
I screwed this up in the previous (post 1.2.16) commits --- src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK -- although I'll admit the construct is painful to read...
John
Thanks, pushed this and patch #4 since they are independent. I'll repost the remaining patches - Cole
-1 is just an internal placeholder and is meaningless to output in the XML. --- docs/schemas/storagecommon.rng | 28 ++++++++++++---------- src/conf/storage_conf.c | 23 +++++++++++------- tests/storagepoolxml2xmlout/pool-dir-naming.xml | 2 -- tests/storagepoolxml2xmlout/pool-dir.xml | 2 -- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 -- .../vol-gluster-dir-neg-uid.xml | 2 -- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 -- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 -- 8 files changed, 30 insertions(+), 33 deletions(-) diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 5f71b10..6f7d369 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -101,18 +101,22 @@ <element name='mode'> <ref name='octalMode'/> </element> - <element name='owner'> - <choice> - <ref name='unsignedInt'/> - <value>-1</value> - </choice> - </element> - <element name='group'> - <choice> - <ref name='unsignedInt'/> - <value>-1</value> - </choice> - </element> + <optional> + <element name='owner'> + <choice> + <ref name='unsignedInt'/> + <value>-1</value> + </choice> + </element> + </optional> + <optional> + <element name='group'> + <choice> + <ref name='unsignedInt'/> + <value>-1</value> + </choice> + </element> + </optional> <optional> <element name='label'> <text/> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4852dfb..99962b7 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -759,6 +759,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (virXPathNode("./owner", ctxt) == NULL) { perms->uid = (uid_t) -1; } else { + /* We previously could output -1, so continue to parse it */ if (virXPathLongLong("number(./owner)", ctxt, &val) < 0 || ((uid_t)val != val && val != -1)) { @@ -773,6 +774,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { + /* We previously could output -1, so continue to parse it */ if (virXPathLongLong("number(./group)", ctxt, &val) < 0 || ((gid_t) val != val && val != -1)) { @@ -1187,10 +1189,12 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<mode>0%o</mode>\n", def->target.perms.mode); - virBufferAsprintf(buf, "<owner>%d</owner>\n", - (int) def->target.perms.uid); - virBufferAsprintf(buf, "<group>%d</group>\n", - (int) def->target.perms.gid); + if (def->target.perms.uid != (uid_t) -1) + virBufferAsprintf(buf, "<owner>%d</owner>\n", + (int) def->target.perms.uid); + if (def->target.perms.gid != (gid_t) -1) + virBufferAsprintf(buf, "<group>%d</group>\n", + (int) def->target.perms.gid); virBufferEscapeString(buf, "<label>%s</label>\n", def->target.perms.label); @@ -1522,11 +1526,12 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAsprintf(buf, "<mode>0%o</mode>\n", def->perms->mode); - virBufferAsprintf(buf, "<owner>%d</owner>\n", - (int) def->perms->uid); - virBufferAsprintf(buf, "<group>%d</group>\n", - (int) def->perms->gid); - + if (def->perms->uid != (uid_t) -1) + virBufferAsprintf(buf, "<owner>%d</owner>\n", + (int) def->perms->uid); + if (def->perms->gid != (gid_t) -1) + virBufferAsprintf(buf, "<group>%d</group>\n", + (int) def->perms->gid); virBufferEscapeString(buf, "<label>%s</label>\n", def->perms->label); diff --git a/tests/storagepoolxml2xmlout/pool-dir-naming.xml b/tests/storagepoolxml2xmlout/pool-dir-naming.xml index 536f58c..dd6d9b8 100644 --- a/tests/storagepoolxml2xmlout/pool-dir-naming.xml +++ b/tests/storagepoolxml2xmlout/pool-dir-naming.xml @@ -10,8 +10,6 @@ <path>/var/lib/libvirt/<images></path> <permissions> <mode>0700</mode> - <owner>-1</owner> - <group>-1</group> <label>some_label_t</label> </permissions> </target> diff --git a/tests/storagepoolxml2xmlout/pool-dir.xml b/tests/storagepoolxml2xmlout/pool-dir.xml index f81bc1d..2054871 100644 --- a/tests/storagepoolxml2xmlout/pool-dir.xml +++ b/tests/storagepoolxml2xmlout/pool-dir.xml @@ -10,8 +10,6 @@ <path>/var/lib/libvirt/images</path> <permissions> <mode>0700</mode> - <owner>-1</owner> - <group>-1</group> <label>some_label_t</label> </permissions> </target> diff --git a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml index bab2a15..90143f9 100644 --- a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml +++ b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml @@ -13,8 +13,6 @@ <path>/mnt/gluster</path> <permissions> <mode>0755</mode> - <owner>-1</owner> - <group>-1</group> </permissions> </target> </pool> diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir-neg-uid.xml b/tests/storagevolxml2xmlout/vol-gluster-dir-neg-uid.xml index 538b31d..0af0be1 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir-neg-uid.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir-neg-uid.xml @@ -10,8 +10,6 @@ <format type='dir'/> <permissions> <mode>0600</mode> - <owner>-1</owner> - <group>-1</group> </permissions> </target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml index 538b31d..0af0be1 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml @@ -10,8 +10,6 @@ <format type='dir'/> <permissions> <mode>0600</mode> - <owner>-1</owner> - <group>-1</group> </permissions> </target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index 0a1f32c..d8f34d3 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -9,8 +9,6 @@ <format type='unknown'/> <permissions> <mode>0600</mode> - <owner>-1</owner> - <group>-1</group> </permissions> </target> </volume> -- 2.4.0
On 05/05/2015 12:44 PM, Cole Robinson wrote:
-1 is just an internal placeholder and is meaningless to output in the XML. --- docs/schemas/storagecommon.rng | 28 ++++++++++++---------- src/conf/storage_conf.c | 23 +++++++++++------- tests/storagepoolxml2xmlout/pool-dir-naming.xml | 2 -- tests/storagepoolxml2xmlout/pool-dir.xml | 2 -- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 -- .../vol-gluster-dir-neg-uid.xml | 2 -- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 -- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 -- 8 files changed, 30 insertions(+), 33 deletions(-)
Might be nice to update formatstorage.html.in to indicate that the <owner> && <group> are optional and if not provided the pool/volume will take the owner/group of the parent directory ACK - John
On 05/08/2015 10:04 AM, John Ferlan wrote:
On 05/05/2015 12:44 PM, Cole Robinson wrote:
-1 is just an internal placeholder and is meaningless to output in the XML. --- docs/schemas/storagecommon.rng | 28 ++++++++++++---------- src/conf/storage_conf.c | 23 +++++++++++------- tests/storagepoolxml2xmlout/pool-dir-naming.xml | 2 -- tests/storagepoolxml2xmlout/pool-dir.xml | 2 -- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 -- .../vol-gluster-dir-neg-uid.xml | 2 -- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 -- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 -- 8 files changed, 30 insertions(+), 33 deletions(-)
Might be nice to update formatstorage.html.in to indicate that the <owner> && <group> are optional and if not provided the pool/volume will take the owner/group of the parent directory
ACK -
Thanks, pushed now. I'll send the docs patch separately since it's a pre-existing issue - Cole
The XML parser sets a default <mode> if none is explicitly passed in. This is then used at pool/vol creation time, and unconditionally reported in the XML. The problem with this approach is that it's impossible for other code to determine if the user explicitly requested a storage mode. There are some cases where we want to make this distinction, but we currently can't. Handle <mode> parsing like we handle <owner>/<group>: if no value is passed in, set it to -1, and adjust the internal consumers to handle it. --- docs/schemas/storagecommon.rng | 8 +++-- src/conf/storage_conf.c | 34 +++++++++------------- src/storage/storage_backend.c | 20 +++++++++---- src/storage/storage_backend.h | 3 ++ src/storage/storage_backend_fs.c | 9 ++++-- src/storage/storage_backend_logical.c | 4 ++- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 1 - tests/storagevolxml2xmlin/vol-file.xml | 4 +-- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 1 - tests/storagevolxml2xmlout/vol-sheepdog.xml | 1 - 10 files changed, 49 insertions(+), 36 deletions(-) diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 6f7d369..7c04462 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -98,9 +98,11 @@ <optional> <element name='permissions'> <interleave> - <element name='mode'> - <ref name='octalMode'/> - </element> + <optional> + <element name='mode'> + <ref name='octalMode'/> + </element> + </optional> <optional> <element name='owner'> <choice> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 99962b7..dae8fc9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -50,9 +50,6 @@ VIR_LOG_INIT("conf.storage_conf"); -#define DEFAULT_POOL_PERM_MODE 0755 -#define DEFAULT_VOL_PERM_MODE 0600 - VIR_ENUM_IMPL(virStorageVol, VIR_STORAGE_VOL_LAST, "file", "block", "dir", "network", "netdir") @@ -718,8 +715,7 @@ virStoragePoolDefParseSourceString(const char *srcSpec, static int virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePermsPtr perms, - const char *permxpath, - int defaultmode) + const char *permxpath) { char *mode; long long val; @@ -730,7 +726,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, node = virXPathNode(permxpath, ctxt); if (node == NULL) { /* Set default values if there is not <permissions> element */ - perms->mode = defaultmode; + perms->mode = (mode_t) -1; perms->uid = (uid_t) -1; perms->gid = (gid_t) -1; perms->label = NULL; @@ -740,10 +736,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, relnode = ctxt->node; ctxt->node = node; - mode = virXPathString("string(./mode)", ctxt); - if (!mode) { - perms->mode = defaultmode; - } else { + if ((mode = virXPathString("string(./mode)", ctxt))) { int tmp; if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { @@ -754,6 +747,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } perms->mode = tmp; VIR_FREE(mode); + } else { + perms->mode = (mode_t) -1; } if (virXPathNode("./owner", ctxt) == NULL) { @@ -949,8 +944,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; if (virStorageDefParsePerms(ctxt, &ret->target.perms, - "./target/permissions", - DEFAULT_POOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error; } @@ -1187,8 +1181,9 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->target.perms.mode); + if (def->target.perms.mode != (mode_t) -1) + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->target.perms.mode); if (def->target.perms.uid != (uid_t) -1) virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->target.perms.uid); @@ -1319,8 +1314,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(ret->target.backingStore->perms) < 0) goto error; if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, - "./backingStore/permissions", - DEFAULT_VOL_PERM_MODE) < 0) + "./backingStore/permissions") < 0) goto error; } @@ -1365,8 +1359,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(ret->target.perms) < 0) goto error; if (virStorageDefParsePerms(ctxt, ret->target.perms, - "./target/permissions", - DEFAULT_VOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error; node = virXPathNode("./target/encryption", ctxt); @@ -1524,8 +1517,9 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->perms->mode); + if (def->perms->mode != (mode_t) -1) + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->perms->mode); if (def->perms->uid != (uid_t) -1) virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->perms->uid); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 289f454..ce59f63 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -318,6 +318,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, struct stat st; gid_t gid; uid_t uid; + mode_t mode; bool reflink_copy = false; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | @@ -367,10 +368,13 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, (unsigned int) gid); goto cleanup; } - if (fchmod(fd, vol->target.perms->mode) < 0) { + + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + if (fchmod(fd, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms->mode); + vol->target.path, mode); goto cleanup; } if (VIR_CLOSE(fd) < 0) { @@ -509,7 +513,9 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms->mode, + (vol->target.perms->mode ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode), vol->target.perms->uid, vol->target.perms->gid, operation_flags)) < 0) { @@ -664,6 +670,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, struct stat st; gid_t gid; uid_t uid; + mode_t mode; bool filecreated = false; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -709,10 +716,13 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, (unsigned int) gid); return -1; } - if (chmod(vol->target.path, vol->target.perms->mode) < 0) { + + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + if (chmod(vol->target.path, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms->mode); + vol->target.path, mode); return -1; } return 0; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 85a8a4b..39c00b1 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -177,6 +177,9 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755 +# define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600 + int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withBlockVolFormat, unsigned int openflags); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 9d84a38..62949ba 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -801,7 +801,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, * 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, + (pool->def->target.perms.mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_POOL_PERM_MODE : + pool->def->target.perms.mode), pool->def->target.perms.uid, pool->def->target.perms.gid, VIR_DIR_CREATE_FORCE_PERMS | @@ -1072,7 +1074,10 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, } - if ((err = virDirCreate(vol->target.path, vol->target.perms->mode, + if ((err = virDirCreate(vol->target.path, + (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode), vol->target.perms->uid, vol->target.perms->gid, VIR_DIR_CREATE_FORCE_PERMS | diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 11c5683..9c77b4c 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -787,7 +787,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, goto error; } } - if (fchmod(fd, vol->target.perms->mode) < 0) { + if (fchmod(fd, (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode)) < 0) { virReportSystemError(errno, _("cannot set file mode '%s'"), vol->target.path); diff --git a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml index 90143f9..9e36cb6 100644 --- a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml +++ b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml @@ -12,7 +12,6 @@ <target> <path>/mnt/gluster</path> <permissions> - <mode>0755</mode> </permissions> </target> </pool> diff --git a/tests/storagevolxml2xmlin/vol-file.xml b/tests/storagevolxml2xmlin/vol-file.xml index d3f65f6..8bb9004 100644 --- a/tests/storagevolxml2xmlin/vol-file.xml +++ b/tests/storagevolxml2xmlin/vol-file.xml @@ -6,8 +6,8 @@ <target> <path>/var/lib/libvirt/images/sparse.img</path> <permissions> - <mode>0</mode> - <owner>0744</owner> + <mode>00</mode> + <owner>744</owner> <group>0</group> <label>virt_image_t</label> </permissions> diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml index 0af0be1..37400b9 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml @@ -9,7 +9,6 @@ <path>gluster://example.com/vol/dir</path> <format type='dir'/> <permissions> - <mode>0600</mode> </permissions> </target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index d8f34d3..fe1879f 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -8,7 +8,6 @@ <path>sheepdog:test2</path> <format type='unknown'/> <permissions> - <mode>0600</mode> </permissions> </target> </volume> -- 2.4.0
On 05/05/2015 12:44 PM, Cole Robinson wrote:
The XML parser sets a default <mode> if none is explicitly passed in. This is then used at pool/vol creation time, and unconditionally reported in the XML.
The problem with this approach is that it's impossible for other code to determine if the user explicitly requested a storage mode. There are some cases where we want to make this distinction, but we currently can't.
Handle <mode> parsing like we handle <owner>/<group>: if no value is passed in, set it to -1, and adjust the internal consumers to handle it. --- docs/schemas/storagecommon.rng | 8 +++-- src/conf/storage_conf.c | 34 +++++++++------------- src/storage/storage_backend.c | 20 +++++++++---- src/storage/storage_backend.h | 3 ++ src/storage/storage_backend_fs.c | 9 ++++-- src/storage/storage_backend_logical.c | 4 ++- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 1 - tests/storagevolxml2xmlin/vol-file.xml | 4 +-- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 1 - tests/storagevolxml2xmlout/vol-sheepdog.xml | 1 - 10 files changed, 49 insertions(+), 36 deletions(-)
Similar to 2/5 a note about <mode> being optional. And in fact <permissions> over is optional...
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 6f7d369..7c04462 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -98,9 +98,11 @@ <optional> <element name='permissions'> <interleave> - <element name='mode'> - <ref name='octalMode'/> - </element> + <optional> + <element name='mode'> + <ref name='octalMode'/> + </element> + </optional> <optional> <element name='owner'> <choice> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 99962b7..dae8fc9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -50,9 +50,6 @@
VIR_LOG_INIT("conf.storage_conf");
-#define DEFAULT_POOL_PERM_MODE 0755 -#define DEFAULT_VOL_PERM_MODE 0600 - VIR_ENUM_IMPL(virStorageVol, VIR_STORAGE_VOL_LAST, "file", "block", "dir", "network", "netdir") @@ -718,8 +715,7 @@ virStoragePoolDefParseSourceString(const char *srcSpec, static int virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePermsPtr perms, - const char *permxpath, - int defaultmode) + const char *permxpath) { char *mode; long long val; @@ -730,7 +726,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, node = virXPathNode(permxpath, ctxt); if (node == NULL) { /* Set default values if there is not <permissions> element */ - perms->mode = defaultmode; + perms->mode = (mode_t) -1; perms->uid = (uid_t) -1; perms->gid = (gid_t) -1; perms->label = NULL; @@ -740,10 +736,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, relnode = ctxt->node; ctxt->node = node;
- mode = virXPathString("string(./mode)", ctxt); - if (!mode) { - perms->mode = defaultmode; - } else { + if ((mode = virXPathString("string(./mode)", ctxt))) { int tmp;
if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { @@ -754,6 +747,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } perms->mode = tmp; VIR_FREE(mode); + } else { + perms->mode = (mode_t) -1; }
if (virXPathNode("./owner", ctxt) == NULL) { @@ -949,8 +944,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error;
if (virStorageDefParsePerms(ctxt, &ret->target.perms, - "./target/permissions", - DEFAULT_POOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error; }
@@ -1187,8 +1181,9 @@ virStoragePoolDefFormatBuf(virBufferPtr buf,
I think right about here there needs to be some logic added to avoid printing (since it's optional anyway): <permissions> </permissions> in the output XML
virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->target.perms.mode); + if (def->target.perms.mode != (mode_t) -1) + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->target.perms.mode); if (def->target.perms.uid != (uid_t) -1) virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->target.perms.uid);
BUT be careful to not lose the </target> printing...
@@ -1319,8 +1314,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(ret->target.backingStore->perms) < 0) goto error; if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, - "./backingStore/permissions", - DEFAULT_VOL_PERM_MODE) < 0) + "./backingStore/permissions") < 0) goto error; }
@@ -1365,8 +1359,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(ret->target.perms) < 0) goto error; if (virStorageDefParsePerms(ctxt, ret->target.perms, - "./target/permissions", - DEFAULT_VOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error;
node = virXPathNode("./target/encryption", ctxt); @@ -1524,8 +1517,9 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
Likewise, I think right about here as a condition to this block we need a way to avoid printing (since it's optional anyway): <permissions> </permissions> in the output XML
virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2);
- virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->perms->mode); + if (def->perms->mode != (mode_t) -1) + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->perms->mode); if (def->perms->uid != (uid_t) -1) virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->perms->uid); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 289f454..ce59f63 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -318,6 +318,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, struct stat st; gid_t gid; uid_t uid; + mode_t mode; bool reflink_copy = false;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | @@ -367,10 +368,13 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, (unsigned int) gid); goto cleanup; } - if (fchmod(fd, vol->target.perms->mode) < 0) { + + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + if (fchmod(fd, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms->mode); + vol->target.path, mode); goto cleanup; } if (VIR_CLOSE(fd) < 0) { @@ -509,7 +513,9 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms->mode, + (vol->target.perms->mode ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode), vol->target.perms->uid, vol->target.perms->gid, operation_flags)) < 0) { @@ -664,6 +670,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, struct stat st; gid_t gid; uid_t uid; + mode_t mode; bool filecreated = false;
if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -709,10 +716,13 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, (unsigned int) gid); return -1; } - if (chmod(vol->target.path, vol->target.perms->mode) < 0) { + + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + if (chmod(vol->target.path, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms->mode); + vol->target.path, mode); return -1; } return 0; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 85a8a4b..39c00b1 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -177,6 +177,9 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755 +# define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600 + int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withBlockVolFormat, unsigned int openflags); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 9d84a38..62949ba 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -801,7 +801,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, * 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, + (pool->def->target.perms.mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_POOL_PERM_MODE : + pool->def->target.perms.mode), pool->def->target.perms.uid, pool->def->target.perms.gid, VIR_DIR_CREATE_FORCE_PERMS | @@ -1072,7 +1074,10 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, }
- if ((err = virDirCreate(vol->target.path, vol->target.perms->mode, + if ((err = virDirCreate(vol->target.path, + (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode), vol->target.perms->uid, vol->target.perms->gid, VIR_DIR_CREATE_FORCE_PERMS | diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 11c5683..9c77b4c 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -787,7 +787,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, goto error; } } - if (fchmod(fd, vol->target.perms->mode) < 0) { + if (fchmod(fd, (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode)) < 0) { virReportSystemError(errno, _("cannot set file mode '%s'"), vol->target.path); diff --git a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml index 90143f9..9e36cb6 100644 --- a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml +++ b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml @@ -12,7 +12,6 @@ <target> <path>/mnt/gluster</path> <permissions> - <mode>0755</mode> </permissions> </target> </pool> diff --git a/tests/storagevolxml2xmlin/vol-file.xml b/tests/storagevolxml2xmlin/vol-file.xml index d3f65f6..8bb9004 100644 --- a/tests/storagevolxml2xmlin/vol-file.xml +++ b/tests/storagevolxml2xmlin/vol-file.xml @@ -6,8 +6,8 @@ <target> <path>/var/lib/libvirt/images/sparse.img</path> <permissions> - <mode>0</mode> - <owner>0744</owner> + <mode>00</mode>
^^^ This one's really odd. Still scratching my head over why previously we printed "<mode>0</mode>", but now we print "<mode>00</mode>"
+ <owner>744</owner> <group>0</group> <label>virt_image_t</label> </permissions> diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml index 0af0be1..37400b9 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml @@ -9,7 +9,6 @@ <path>gluster://example.com/vol/dir</path> <format type='dir'/> <permissions> - <mode>0600</mode> </permissions>
This will need updating to remove the unnecessary <permissions> </permissions>
</target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index d8f34d3..fe1879f 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -8,7 +8,6 @@ <path>sheepdog:test2</path> <format type='unknown'/> <permissions> - <mode>0600</mode> </permissions>
This will need updating too ACK with the adjustments - any maybe you can chase why we get "00" now... John
</target> </volume>
On 05/08/2015 08:24 AM, John Ferlan wrote:
I think right about here there needs to be some logic added to avoid printing (since it's optional anyway):
<permissions> </permissions>
in the output XML
We may still want to output: <permissions/> to make it obvious that we considered permissions, but had nothing to say about them (as opposed to no information about it at all) - similar to how <backingChain/> is used in <disk> elements to distinguish between end-of-chain and no information available. But I agree that an empty <permissions> </permissions> looks weird, even though it is equally valid as XML. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, May 05, 2015 at 12:44:01PM -0400, Cole Robinson wrote:
The XML parser sets a default <mode> if none is explicitly passed in. This is then used at pool/vol creation time, and unconditionally reported in the XML.
The problem with this approach is that it's impossible for other code to determine if the user explicitly requested a storage mode. There are some cases where we want to make this distinction, but we currently can't.
Handle <mode> parsing like we handle <owner>/<group>: if no value is passed in, set it to -1, and adjust the internal consumers to handle it.
--- a/tests/storagevolxml2xmlin/vol-file.xml +++ b/tests/storagevolxml2xmlin/vol-file.xml @@ -6,8 +6,8 @@ <target> <path>/var/lib/libvirt/images/sparse.img</path> <permissions> - <mode>0</mode> - <owner>0744</owner> + <mode>00</mode>
00 is ugly.
+ <owner>744</owner>
Why are these changes needed? The patch does not touch the owner formatting. Jan
<group>0</group> <label>virt_image_t</label> </permissions>
On 05/11/2015 04:10 AM, Ján Tomko wrote:
On Tue, May 05, 2015 at 12:44:01PM -0400, Cole Robinson wrote:
The XML parser sets a default <mode> if none is explicitly passed in. This is then used at pool/vol creation time, and unconditionally reported in the XML.
The problem with this approach is that it's impossible for other code to determine if the user explicitly requested a storage mode. There are some cases where we want to make this distinction, but we currently can't.
Handle <mode> parsing like we handle <owner>/<group>: if no value is passed in, set it to -1, and adjust the internal consumers to handle it.
--- a/tests/storagevolxml2xmlin/vol-file.xml +++ b/tests/storagevolxml2xmlin/vol-file.xml @@ -6,8 +6,8 @@ <target> <path>/var/lib/libvirt/images/sparse.img</path> <permissions> - <mode>0</mode> - <owner>0744</owner> + <mode>00</mode>
00 is ugly.
+ <owner>744</owner>
Why are these changes needed? The patch does not touch the owner formatting.
Yeah I think this is some weirdness left over from v1 of the series, will fix - Cole
The only two virDirCreate callers already use it --- src/storage/storage_backend_fs.c | 2 -- src/util/virfile.c | 6 ++---- src/util/virfile.h | 3 +-- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 62949ba..ed56935 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -806,7 +806,6 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, pool->def->target.perms.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) { @@ -1080,7 +1079,6 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.perms->mode), vol->target.perms->uid, vol->target.perms->gid, - VIR_DIR_CREATE_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS ? VIR_DIR_CREATE_AS_UID : 0))) < 0) { return -1; diff --git a/src/util/virfile.c b/src/util/virfile.c index 6e9ecbe..63eafdf 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,8 +2311,7 @@ virDirCreateNoFork(const char *path, path, (unsigned int) uid, (unsigned int) gid); goto error; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) - && (chmod(path, mode) < 0)) { + if (chmod(path, mode) < 0) { ret = -errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), @@ -2425,8 +2424,7 @@ virDirCreate(const char *path, path, (unsigned int) gid); goto childerror; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) - && chmod(path, mode) < 0) { + if (chmod(path, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), path, mode); diff --git a/src/util/virfile.h b/src/util/virfile.h index 403d0ba..2d27e89 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -223,8 +223,7 @@ int virFileOpenAs(const char *path, int openflags, mode_t mode, enum { VIR_DIR_CREATE_NONE = 0, VIR_DIR_CREATE_AS_UID = (1 << 0), - VIR_DIR_CREATE_FORCE_PERMS = (1 << 1), - VIR_DIR_CREATE_ALLOW_EXIST = (1 << 2), + VIR_DIR_CREATE_ALLOW_EXIST = (1 << 1), }; int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) ATTRIBUTE_RETURN_CHECK; -- 2.4.0
$SUBJ: s/redundand/redundant On 05/05/2015 12:44 PM, Cole Robinson wrote:
The only two virDirCreate callers already use it --- src/storage/storage_backend_fs.c | 2 -- src/util/virfile.c | 6 ++---- src/util/virfile.h | 3 +-- 3 files changed, 3 insertions(+), 8 deletions(-)
Seems this is/was a holdover when directory and file operations were intermixed.... ACK John
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(-) 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); -- 2.4.0
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);
participants (4)
-
Cole Robinson -
Eric Blake -
John Ferlan -
Ján Tomko