- Don't redocument the permissions fields for backingstore, just point to the volume docs. - Clarify that owner/group are inherited from the parent directory at volume create/pool build time. - Clarify that <permissions> fields report runtime values too --- v3: New patch docs/formatstorage.html.in | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 474abd6..f07bb5d 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -405,11 +405,17 @@ pools, which are mapped as a directory into the local filesystem 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. The - <code>owner</code> element contains the numeric user ID. The <code>group</code> - element contains the numeric group ID. The <code>label</code> element - contains the MAC (eg SELinux) label string. + <code>mode</code> element contains the octal permission set. + 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 + creating a directory, the values are inherited from the parent + directory. The <code>label</code> element contains the MAC (eg SELinux) + label string. <span class="since">Since 0.4.1</span> + For running directory or filesystem based pools, these fields + will be filled with the values used by the existing directory. + <span class="since">Since 1.2.16</span> </dd> <dt><code>timestamps</code></dt> <dd>Provides timing information about the volume. Up to four @@ -583,15 +589,20 @@ volume format type value and the default pool format will be used. <span class="since">Since 0.4.1</span></dd> <dt><code>permissions</code></dt> - <dd>Provides information about the default permissions to use + <dd>Provides information about the permissions to use when creating volumes. This is currently only useful for directory or filesystem based pools, where the volumes allocated are simple 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. The - <code>owner</code> element contains the numeric user ID. The <code>group</code> - element contains the numeric group ID. The <code>label</code> element - contains the MAC (eg SELinux) label string. + <code>mode</code> element contains the octal permission set. + 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 + creating a supported volume, the values are inherited from the parent + directory. The <code>label</code> element contains the MAC (eg SELinux) + label string. + For existing directory or filesystem based volumes, these fields + will be filled with the values used by the existing file. <span class="since">Since 0.4.1</span> </dd> <dt><code>compat</code></dt> @@ -659,11 +670,8 @@ <span class="since">Since 0.6.0</span></dd> <dt><code>permissions</code></dt> <dd>Provides information about the permissions of the backing file. - It contains 4 child elements. The - <code>mode</code> element contains the octal permission set. The - <code>owner</code> element contains the numeric user ID. The <code>group</code> - element contains the numeric group ID. The <code>label</code> element - contains the MAC (eg SELinux) label string. + See volume <code>permissions</code> documentation for explanation + of individual fields. <span class="since">Since 0.6.0</span> </dd> </dl> -- 2.4.1

On 05/24/2015 07:20 AM, John Ferlan wrote: Fixed. Fixed. This was intentional, the pool permission syncing pre-dates 1.2.16, my patches only added it for volumes. I tried a git log grep to try and figure out when it was added but gave up after a couple minutes. So I left this as is and pushed. Thanks, 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. --- 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> -- 2.4.1

On 05/24/2015 07:37 AM, John Ferlan wrote: Thanks, fixed. It's already saying 'see volume docs' essentially, so this should be fine. Pushed with the change mentioned above. Thanks, Cole

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. --- v3: Minor code clarity tweaks Drop an unused variable while I'm here src/storage/storage_backend_fs.c | 29 ++++++++++++++++++----------- src/util/virfile.c | 4 ++-- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ed56935..bcbbb3a 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -766,9 +766,11 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, unsigned int flags) { - int err, ret = -1; + int ret = -1; char *parent = NULL; char *p = NULL; + mode_t mode; + bool needs_create_as_uid, dir_create_flags; virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); @@ -797,20 +799,25 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, } } + dir_create_flags = VIR_DIR_CREATE_ALLOW_EXIST; + 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; + if (needs_create_as_uid) + flags |= VIR_DIR_CREATE_AS_UID; + /* 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), - 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) { + if (virDirCreate(pool->def->target.path, + mode, + pool->def->target.perms.uid, + pool->def->target.perms.gid, + dir_create_flags) < 0) goto error; - } if (flags != 0) { ret = virStorageBackendMakeFileSystem(pool, flags); 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.1