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

Reply via email to