[PATCH v3 2/4] storage: conf: Don't set any default <mode> in the XML

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

 



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.
---
v3:
    Drop needless test churn
    Add docs about default <mode>

 docs/formatstorage.html.in                         |  4 +++
 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/storagevolxml2xmlout/vol-gluster-dir.xml     |  1 -
 tests/storagevolxml2xmlout/vol-sheepdog.xml        |  1 -
 10 files changed, 51 insertions(+), 34 deletions(-)

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index f07bb5d..ccde978 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -406,6 +406,8 @@
         namespace. It provides information about the permissions to use for the
         final directory when the pool is built. The
         <code>mode</code> element contains the octal permission set.
+        If <code>mode</code> is unset when creating a directory,
+        the value 0755 is used.
         The <code>owner</code> element contains the numeric user ID.
         The <code>group</code> element contains the numeric group ID.
         If <code>owner</code> or <code>group</code> aren't specified when
@@ -595,6 +597,8 @@
         files. For pools where the volumes are device nodes, the hotplug
         scripts determine permissions. It contains 4 child elements. The
         <code>mode</code> element contains the octal permission set.
+        If <code>mode</code> is unset when creating a supported volume,
+        the value 0600 is used.
         The <code>owner</code> element contains the numeric user ID.
         The <code>group</code> element contains the numeric group ID.
         If <code>owner</code> or <code>group</code> aren't specified when
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 ee6e0cf..a02e504 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,
 
         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);
@@ -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,
         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 235ab20..ed56935 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_ALLOW_EXIST |
@@ -1071,7 +1073,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,
                             (pool->def->type == VIR_STORAGE_POOL_NETFS
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/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>
   </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>
   </target>
 </volume>
-- 
2.4.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]