[libvirt] [PATCH v3 0/4] 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 v2 of the series. Now the patches are: - Patch 1 (new) update existing docs about storage <permissions> - Patch 2 changes storage XML to not hardcode a <mode> value if the user didn't specify one in. - Patch 3 (new) don't output empty <permissions> block - Patch 4 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. Cole Robinson (4): docs: formatstorage: Update <permissions> docs storage: conf: Don't set any default <mode> in the XML conf: storage: Don't emit empty <permissions> block storage: fs: Only force directory permissions if required docs/formatstorage.html.in | 40 ++++++++----- docs/schemas/storagecommon.rng | 8 ++- src/conf/storage_conf.c | 70 ++++++++++++---------- src/storage/storage_backend.c | 20 +++++-- src/storage/storage_backend.h | 3 + src/storage/storage_backend_fs.c | 32 ++++++---- src/storage/storage_backend_logical.c | 4 +- src/util/virfile.c | 4 +- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 3 - tests/storagevolxml2xmlout/vol-gluster-dir.xml | 3 - tests/storagevolxml2xmlout/vol-sheepdog.xml | 3 - 11 files changed, 113 insertions(+), 77 deletions(-) -- 2.4.1
- 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/21/2015 04:03 PM, Cole Robinson wrote:
- 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.
s/.$/. There are 4 child elements.
+ <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. ^^^
s/It contains /There are/ the <span> used above for 1.2.16 ^^^ the <span> used above for 1.2.16
<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>
On 05/24/2015 07:20 AM, John Ferlan wrote:
On 05/21/2015 04:03 PM, Cole Robinson wrote:
- 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.
s/.$/. There are 4 child elements.
Fixed.
+ <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
s/It contains /There are/
Fixed.
- <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. ^^^ the <span> used above for 1.2.16 ^^^ the <span> used above for 1.2.16
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
<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>
On 05/21/2015 04:03 PM, Cole Robinson wrote:
- 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
For some reason - I sent it too quick...
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.
I was trying to note these two lines don't have the <span> like the <source pool>... and they should probably be moved after the following <span> to be consistent.
<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.
It's too bad the formatstorage.html doesn't have the section tags like a few other places so an href could point one at the <source pool> <target> <permissions> or the <source volume> <target> <permissions> ACK John
<span class="since">Since 0.6.0</span> </dd> </dl>
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/21/2015 04:03 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. --- 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.
Consider "The mode defaults to 0755 when not provided." The verbiage "is unset" reads strangly
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.
ditto but 0600 Also, wherever you "point" the "<backing store elements> <permissions> to should be whichever default is correct for the backing store. that is from patch 1, you indicated that the elements are the same as one of these two elements, so to be technically correct be sure to redirect to either the 0755 or 0600 for which the backing store element would default to if not provided. ACK John
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>
On 05/24/2015 07:37 AM, John Ferlan wrote:
On 05/21/2015 04:03 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. --- 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.
Consider "The mode defaults to 0755 when not provided."
The verbiage "is unset" reads strangly
Thanks, fixed.
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.
ditto but 0600
Also, wherever you "point" the "<backing store elements> <permissions> to should be whichever default is correct for the backing store. that is from patch 1, you indicated that the elements are the same as one of these two elements, so to be technically correct be sure to redirect to either the 0755 or 0600 for which the backing store element would default to if not provided.
It's already saying 'see volume docs' essentially, so this should be fine. Pushed with the change mentioned above. Thanks, Cole
--- v3: New patch src/conf/storage_conf.c | 42 +++++++++++++--------- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 -- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 -- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 -- 4 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a02e504..7857a5e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1179,22 +1179,28 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, virBufferEscapeString(buf, "<path>%s</path>\n", def->target.path); - virBufferAddLit(buf, "<permissions>\n"); - virBufferAdjustIndent(buf, 2); - 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); - 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); + if (def->target.perms.mode != (mode_t) -1 || + def->target.perms.uid != (uid_t) -1 || + def->target.perms.gid != (gid_t) -1 || + def->target.perms.label) { + virBufferAddLit(buf, "<permissions>\n"); + virBufferAdjustIndent(buf, 2); + 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); + 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); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</permissions>\n"); + } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</permissions>\n"); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</target>\n"); } @@ -1513,7 +1519,11 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAsprintf(buf, "<format type='%s'/>\n", format); } - if (def->perms) { + if (def->perms && + (def->perms->mode != (mode_t) -1 || + def->perms->uid != (uid_t) -1 || + def->perms->gid != (gid_t) -1 || + def->perms->label)) { virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); diff --git a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml index 9e36cb6..dd7ffb5 100644 --- a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml +++ b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml @@ -11,7 +11,5 @@ </source> <target> <path>/mnt/gluster</path> - <permissions> - </permissions> </target> </pool> diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml index 37400b9..d422248 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml @@ -8,7 +8,5 @@ <target> <path>gluster://example.com/vol/dir</path> <format type='dir'/> - <permissions> - </permissions> </target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index fe1879f..e1d6a9e 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -7,7 +7,5 @@ <target> <path>sheepdog:test2</path> <format type='unknown'/> - <permissions> - </permissions> </target> </volume> -- 2.4.1
On 05/21/2015 04:03 PM, Cole Robinson wrote:
--- v3: New patch
src/conf/storage_conf.c | 42 +++++++++++++--------- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 -- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 -- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 -- 4 files changed, 26 insertions(+), 22 deletions(-)
ACK - although I suppose it could be combined with 2/4, but I'm OK with it being separate. John
On 05/24/2015 07:38 AM, John Ferlan wrote:
On 05/21/2015 04:03 PM, Cole Robinson wrote:
--- v3: New patch
src/conf/storage_conf.c | 42 +++++++++++++--------- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 -- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 -- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 -- 4 files changed, 26 insertions(+), 22 deletions(-)
ACK - although I suppose it could be combined with 2/4, but I'm OK with it being separate.
I intentionally kept them separate since I didn't want to mix up functional changes with a cosmetic output change. Pushed this and patch #4 as is. Thanks for the review! - 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
On 05/21/2015 04:03 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. --- 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(-)
ACK John
On 05/21/2015 07:03 PM, Cole Robinson wrote:
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 v2 of the series. Now the patches are:
- Patch 1 (new) update existing docs about storage <permissions> - Patch 2 changes storage XML to not hardcode a <mode> value if the user didn't specify one in. - Patch 3 (new) don't output empty <permissions> block - Patch 4 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.
Cole Robinson (4): docs: formatstorage: Update <permissions> docs storage: conf: Don't set any default <mode> in the XML conf: storage: Don't emit empty <permissions> block storage: fs: Only force directory permissions if required
docs/formatstorage.html.in | 40 ++++++++----- docs/schemas/storagecommon.rng | 8 ++- src/conf/storage_conf.c | 70 ++++++++++++---------- src/storage/storage_backend.c | 20 +++++-- src/storage/storage_backend.h | 3 + src/storage/storage_backend_fs.c | 32 ++++++---- src/storage/storage_backend_logical.c | 4 +- src/util/virfile.c | 4 +- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 3 - tests/storagevolxml2xmlout/vol-gluster-dir.xml | 3 - tests/storagevolxml2xmlout/vol-sheepdog.xml | 3 - 11 files changed, 113 insertions(+), 77 deletions(-)
Ignore this, accidentally resent these patches - Cole
participants (2)
-
Cole Robinson -
John Ferlan