Re: [libvirt] [PATCH 5/6] storage: conf: Don't set any default <mode> in the XML

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.