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>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list