Re: [libvirt] [PATCH v3 2/4] storage: conf: Don't set any default <mode> in the XML

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. --- v3: Drop needless test churn Add docs about default <mode> docs/formatstorage.html.in | 4 +++ 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/storagevolxml2xmlout/vol-gluster-dir.xml | 1 - tests/storagevolxml2xmlout/vol-sheepdog.xml | 1 - 10 files changed, 51 insertions(+), 34 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index f07bb5d..ccde978 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -406,6 +406,8 @@ namespace. It provides information about the permissions to use for the final directory when the pool is built. The <code>mode</code> element contains the octal permission set. + If <code>mode</code> is unset when creating a directory, + the value 0755 is used.

The <code>owner</code> element contains the numeric user ID. The <code>group</code> element contains the numeric group ID. If <code>owner</code> or <code>group</code> aren't specified when @@ -595,6 +597,8 @@ files. For pools where the volumes are device nodes, the hotplug scripts determine permissions. It contains 4 child elements. The <code>mode</code> element contains the octal permission set. + If <code>mode</code> is unset when creating a supported volume, + the value 0600 is used.

The <code>owner</code> element contains the numeric user ID. The <code>group</code> element contains the numeric group ID. If <code>owner</code> or <code>group</code> aren't specified when 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 ee6e0cf..a02e504 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 235ab20..ed56935 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_ALLOW_EXIST | @@ -1071,7 +1073,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, (pool->def->type == VIR_STORAGE_POOL_NETFS 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/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>