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