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

[Peter Krempa]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150428/8a9fc154/attachment-0001.sig>