On Mon, Apr 27, 2015 at 16:48:43 -0400, Cole Robinson wrote:
> The XML parser sets a default <mode> if none is explicitly passed in.
> This is then used at pool/vol creation time, and unconditionally reported
> in the XML.
>
> The problem with this approach is that it's impossible for other code
> to determine if the user explicitly requested a storage mode. There
> are some cases where we want to make this distinction, but we currently
> can't.
>
> Handle <mode> parsing like we handle <owner>/<group>: if no value is
> passed in, set it to -1, and adjust the internal consumers to handle
> it.
> ---
> docs/schemas/storagecommon.rng | 5 ++-
> src/conf/storage_conf.c | 42 +++++++++++-----------
> 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/storagepoolxml2xmlin/pool-dir.xml | 2 +-
> tests/storagepoolxml2xmlout/pool-dir.xml | 2 +-
> tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 +-
> tests/storagevolxml2xmlin/vol-file.xml | 6 ++--
> tests/storagevolxml2xmlout/vol-file.xml | 6 ++--
> tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 +-
> tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 +-
> 13 files changed, 64 insertions(+), 41 deletions(-)
>
> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
> index 5f71b10..e4e8a51 100644
> --- a/docs/schemas/storagecommon.rng
> +++ b/docs/schemas/storagecommon.rng
> @@ -99,7 +99,10 @@
> <element name='permissions'>
> <interleave>
> <element name='mode'>
> - <ref name='octalMode'/>
> + <choice>
> + <ref name='octalMode'/>
> + <value>-1</value>
> + </choice>
I'd rather make the mode optional if you want to keep the default value.
> </element>
> <element name='owner'>
> <choice>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 4852dfb..7131242 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,13 +736,12 @@ 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)) {
> + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 ||
> + ((tmp & ~0777) &&
> + tmp != -1)) {
> VIR_FREE(mode);
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("malformed octal mode"));
> @@ -754,6 +749,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
> }
> perms->mode = tmp;
> VIR_FREE(mode);
> + } else {
> + perms->mode = (mode_t) -1;
> }
>
> if (virXPathNode("./owner", ctxt) == NULL) {
> @@ -947,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;
> }
>
> @@ -1185,8 +1181,11 @@ 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)
> + virBufferAddLit(buf, "<mode>-1</mode>\n");
And I'd skip formatting it.
> + else
> + virBufferAsprintf(buf, "<mode>0%o</mode>\n",
> + def->target.perms.mode);
> virBufferAsprintf(buf, "<owner>%d</owner>\n",
> (int) def->target.perms.uid);
> virBufferAsprintf(buf, "<group>%d</group>\n",
Using -1 looks rather ugly.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list