On 04/28/2015 09:23 AM, Peter Krempa wrote:
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.
Agreed, but I wanted to keep parity with how we handle uid/gid, they will output -1 as well. After the release I'll come back to this and add an extra patch to drop uid/gid outputing -1, then alter this patch to match Thanks, Cole