[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




On 05/05/2015 12:44 PM, 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                     |  8 +++--
>  src/conf/storage_conf.c                            | 34 +++++++++-------------
>  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/storagepoolxml2xmlout/pool-netfs-gluster.xml |  1 -
>  tests/storagevolxml2xmlin/vol-file.xml             |  4 +--
>  tests/storagevolxml2xmlout/vol-gluster-dir.xml     |  1 -
>  tests/storagevolxml2xmlout/vol-sheepdog.xml        |  1 -
>  10 files changed, 49 insertions(+), 36 deletions(-)
> 

Similar to 2/5 a note about <mode> being optional.

And in fact <permissions> over is optional...

> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
> index 6f7d369..7c04462 100644
> --- a/docs/schemas/storagecommon.rng
> +++ b/docs/schemas/storagecommon.rng
> @@ -98,9 +98,11 @@
>      <optional>
>        <element name='permissions'>
>          <interleave>
> -          <element name='mode'>
> -            <ref name='octalMode'/>
> -          </element>
> +          <optional>
> +            <element name='mode'>
> +              <ref name='octalMode'/>
> +            </element>
> +          </optional>
>            <optional>
>              <element name='owner'>
>                <choice>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 99962b7..dae8fc9 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,10 +736,7 @@ 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)) {
> @@ -754,6 +747,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>          }
>          perms->mode = tmp;
>          VIR_FREE(mode);
> +    } else {
> +        perms->mode = (mode_t) -1;
>      }
>  
>      if (virXPathNode("./owner", ctxt) == NULL) {
> @@ -949,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;
>      }
>  
> @@ -1187,8 +1181,9 @@ virStoragePoolDefFormatBuf(virBufferPtr buf,
>  

I think right about here there needs to be some logic added to avoid
printing (since it's optional anyway):

<permissions>
</permissions>

in the output XML

>          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)
> +            virBufferAsprintf(buf, "<mode>0%o</mode>\n",
> +                              def->target.perms.mode);
>          if (def->target.perms.uid != (uid_t) -1)
>              virBufferAsprintf(buf, "<owner>%d</owner>\n",
>                                (int) def->target.perms.uid);


BUT be careful to not lose the </target> printing...

> @@ -1319,8 +1314,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>          if (VIR_ALLOC(ret->target.backingStore->perms) < 0)
>              goto error;
>          if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms,
> -                                    "./backingStore/permissions",
> -                                    DEFAULT_VOL_PERM_MODE) < 0)
> +                                    "./backingStore/permissions") < 0)
>              goto error;
>      }
>  
> @@ -1365,8 +1359,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>      if (VIR_ALLOC(ret->target.perms) < 0)
>          goto error;
>      if (virStorageDefParsePerms(ctxt, ret->target.perms,
> -                                "./target/permissions",
> -                                DEFAULT_VOL_PERM_MODE) < 0)
> +                                "./target/permissions") < 0)
>          goto error;
>  
>      node = virXPathNode("./target/encryption", ctxt);
> @@ -1524,8 +1517,9 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,



Likewise, I think right about here as a condition to this block we need
a way to avoid printing (since it's optional anyway):
<permissions>
</permissions>

in the output XML


>          virBufferAddLit(buf, "<permissions>\n");
>          virBufferAdjustIndent(buf, 2);
>  
> -        virBufferAsprintf(buf, "<mode>0%o</mode>\n",
> -                          def->perms->mode);
> +        if (def->perms->mode != (mode_t) -1)
> +            virBufferAsprintf(buf, "<mode>0%o</mode>\n",
> +                              def->perms->mode);
>          if (def->perms->uid != (uid_t) -1)
>              virBufferAsprintf(buf, "<owner>%d</owner>\n",
>                                (int) def->perms->uid);
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 289f454..ce59f63 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -318,6 +318,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
>      struct stat st;
>      gid_t gid;
>      uid_t uid;
> +    mode_t mode;
>      bool reflink_copy = false;
>  
>      virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA |
> @@ -367,10 +368,13 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
>                               (unsigned int) gid);
>          goto cleanup;
>      }
> -    if (fchmod(fd, vol->target.perms->mode) < 0) {
> +
> +    mode = (vol->target.perms->mode == (mode_t) -1 ?
> +            VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode);
> +    if (fchmod(fd, mode) < 0) {
>          virReportSystemError(errno,
>                               _("cannot set mode of '%s' to %04o"),
> -                             vol->target.path, vol->target.perms->mode);
> +                             vol->target.path, mode);
>          goto cleanup;
>      }
>      if (VIR_CLOSE(fd) < 0) {
> @@ -509,7 +513,9 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
>  
>      if ((fd = virFileOpenAs(vol->target.path,
>                              O_RDWR | O_CREAT | O_EXCL,
> -                            vol->target.perms->mode,
> +                            (vol->target.perms->mode ?
> +                             VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
> +                             vol->target.perms->mode),
>                              vol->target.perms->uid,
>                              vol->target.perms->gid,
>                              operation_flags)) < 0) {
> @@ -664,6 +670,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
>      struct stat st;
>      gid_t gid;
>      uid_t uid;
> +    mode_t mode;
>      bool filecreated = false;
>  
>      if ((pool->def->type == VIR_STORAGE_POOL_NETFS)
> @@ -709,10 +716,13 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
>                               (unsigned int) gid);
>          return -1;
>      }
> -    if (chmod(vol->target.path, vol->target.perms->mode) < 0) {
> +
> +    mode = (vol->target.perms->mode == (mode_t) -1 ?
> +            VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode);
> +    if (chmod(vol->target.path, mode) < 0) {
>          virReportSystemError(errno,
>                               _("cannot set mode of '%s' to %04o"),
> -                             vol->target.path, vol->target.perms->mode);
> +                             vol->target.path, mode);
>          return -1;
>      }
>      return 0;
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 85a8a4b..39c00b1 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -177,6 +177,9 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb,
>      ATTRIBUTE_RETURN_CHECK
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> +# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755
> +# define VIR_STORAGE_DEFAULT_VOL_PERM_MODE  0600
> +
>  int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
>                                     bool withBlockVolFormat,
>                                     unsigned int openflags);
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 9d84a38..62949ba 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -801,7 +801,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>       * requested in the config. If the dir already exists, just set
>       * the perms. */
>      if ((err = virDirCreate(pool->def->target.path,
> -                            pool->def->target.perms.mode,
> +                            (pool->def->target.perms.mode == (mode_t) -1 ?
> +                             VIR_STORAGE_DEFAULT_POOL_PERM_MODE :
> +                             pool->def->target.perms.mode),
>                              pool->def->target.perms.uid,
>                              pool->def->target.perms.gid,
>                              VIR_DIR_CREATE_FORCE_PERMS |
> @@ -1072,7 +1074,10 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
>      }
>  
>  
> -    if ((err = virDirCreate(vol->target.path, vol->target.perms->mode,
> +    if ((err = virDirCreate(vol->target.path,
> +                            (vol->target.perms->mode == (mode_t) -1 ?
> +                             VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
> +                             vol->target.perms->mode),
>                              vol->target.perms->uid,
>                              vol->target.perms->gid,
>                              VIR_DIR_CREATE_FORCE_PERMS |
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 11c5683..9c77b4c 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -787,7 +787,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>              goto error;
>          }
>      }
> -    if (fchmod(fd, vol->target.perms->mode) < 0) {
> +    if (fchmod(fd, (vol->target.perms->mode == (mode_t) -1 ?
> +                    VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
> +                    vol->target.perms->mode)) < 0) {
>          virReportSystemError(errno,
>                               _("cannot set file mode '%s'"),
>                               vol->target.path);
> diff --git a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml
> index 90143f9..9e36cb6 100644
> --- a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml
> +++ b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml
> @@ -12,7 +12,6 @@
>    <target>
>      <path>/mnt/gluster</path>
>      <permissions>
> -      <mode>0755</mode>
>      </permissions>
>    </target>
>  </pool>
> diff --git a/tests/storagevolxml2xmlin/vol-file.xml b/tests/storagevolxml2xmlin/vol-file.xml
> index d3f65f6..8bb9004 100644
> --- a/tests/storagevolxml2xmlin/vol-file.xml
> +++ b/tests/storagevolxml2xmlin/vol-file.xml
> @@ -6,8 +6,8 @@
>    <target>
>      <path>/var/lib/libvirt/images/sparse.img</path>
>      <permissions>
> -      <mode>0</mode>
> -      <owner>0744</owner>
> +      <mode>00</mode>

^^^
This one's really odd. Still scratching my head over why previously we
printed "<mode>0</mode>", but now we print "<mode>00</mode>"


> +      <owner>744</owner>
>        <group>0</group>
>        <label>virt_image_t</label>
>      </permissions>
> diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml
> index 0af0be1..37400b9 100644
> --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml
> +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml
> @@ -9,7 +9,6 @@
>      <path>gluster://example.com/vol/dir</path>
>      <format type='dir'/>
>      <permissions>
> -      <mode>0600</mode>
>      </permissions>

This will need updating to remove the unnecessary

<permissions>
</permissions>

>    </target>
>  </volume>
> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml
> index d8f34d3..fe1879f 100644
> --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml
> +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml
> @@ -8,7 +8,6 @@
>      <path>sheepdog:test2</path>
>      <format type='unknown'/>
>      <permissions>
> -      <mode>0600</mode>
>      </permissions>

This will need updating too

ACK with the adjustments - any maybe you can chase why we get "00" now...

John

>    </target>
>  </volume>
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]