virStorageSource: Eliminate 'volume' field

While historically we've stored the 'pool' and 'image' properties of RBD
and gluster images in separate fields but they are presented in a single
field in the XML. This creates multiple points where they need to be
separated and combined.

Introduce helper 'virStorageSourceNetworkProtocolPathSplit' which will
do that at the point of use rather than everywhere in the code.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0342d6eae7f83ebfe5437363f4c801c05e977c4f..c6ef3ac2062b6dcf110d4a6d4eb4b63d682843bc 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7370,6 +7370,9 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
         return -1;
     }
 
+    if (virStorageSourceNetworkProtocolPathSplit(src->path, src->protocol, NULL, NULL) < 0)
+        return -1;
+
     if (virXMLPropTristateBool(node, "tls", VIR_XML_PROP_NONE,
                                &src->haveTLS) < 0)
         return -1;
@@ -7393,27 +7396,6 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
         }
     }
 
-    /* for historical reasons we store the volume and image name in one XML
-     * element although it complicates thing when attempting to access them. */
-    if (src->path &&
-        (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER ||
-         src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
-        char *tmp;
-        if (!(tmp = strchr(src->path, '/')) ||
-            tmp == src->path) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           _("can't split path '%1$s' into pool name and image name"),
-                           src->path);
-            return -1;
-        }
-
-        src->volume = src->path;
-
-        src->path = g_strdup(tmp + 1);
-
-        tmp[0] = '\0';
-    }
-
     /* snapshot currently works only for remote disks */
     src->snapshot = virXPathString("string(./snapshot/@name)", ctxt);
 
@@ -23155,15 +23137,11 @@ virDomainDiskSourceFormatNetwork(virBuffer *attrBuf,
                                  unsigned int flags)
 {
     size_t n;
-    g_autofree char *path = NULL;
 
     virBufferAsprintf(attrBuf, " protocol='%s'",
                       virStorageNetProtocolTypeToString(src->protocol));
 
-    if (src->volume)
-        path = g_strdup_printf("%s/%s", src->volume, src->path);
-
-    virBufferEscapeString(attrBuf, " name='%s'", path ? path : src->path);
+    virBufferEscapeString(attrBuf, " name='%s'", src->path);
     virBufferEscapeString(attrBuf, " query='%s'", src->query);
 
     if (src->haveTLS != VIR_TRISTATE_BOOL_ABSENT &&

diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
index 8bab116d89a72142f8699283b7e15a036e52111c..2d57e7e816bbbc0572d26e913e63b19c81f15d52 100644 (file)
--- a/src/conf/storage_source_conf.c
+++ b/src/conf/storage_source_conf.c
@@ -820,7 +820,6 @@ virStorageSourceCopy(const virStorageSource *src,
 
     def->path = g_strdup(src->path);
     def->fdgroup = g_strdup(src->fdgroup);
-    def->volume = g_strdup(src->volume);
     def->relPath = g_strdup(src->relPath);
     def->backingStoreRaw = g_strdup(src->backingStoreRaw);
     def->backingStoreRawFormat = src->backingStoreRawFormat;
@@ -946,7 +945,6 @@ virStorageSourceIsSameLocation(virStorageSource *a,
         return false;
 
     if (STRNEQ_NULLABLE(a->path, b->path) ||
-        STRNEQ_NULLABLE(a->volume, b->volume) ||
         STRNEQ_NULLABLE(a->snapshot, b->snapshot))
         return false;
 
@@ -1153,7 +1151,6 @@ virStorageSourceClear(virStorageSource *def)
 
     VIR_FREE(def->path);
     VIR_FREE(def->fdgroup);
-    VIR_FREE(def->volume);
     VIR_FREE(def->vdpadev);
     VIR_FREE(def->snapshot);
     VIR_FREE(def->configFile);
@@ -1447,3 +1444,56 @@ virStorageSourceFDTupleNew(void)
 {
     return g_object_new(vir_storage_source_fd_tuple_get_type(), NULL);
 }
+
+
+/**
+ * virStorageSourceNetworkProtocolPathSplit:
+ * @path: path to split
+ * @protocol: protocol
+ * @pool: filled with pool name (may be NULL)
+ * @image: filled with image name (may be NULL)
+ *
+ * Historically libvirt accepted the specification of Gluster's volume and
+ * RBD's pool as part of the 'path' field, but internally many places require
+ * individual components.
+ *
+ * This helper validates and splits the path as appropriate for given protocol.
+ * It's useful for 'gluster' and 'rbd' protocol but for validation can be called
+ * with any protocol.
+ */
+int
+virStorageSourceNetworkProtocolPathSplit(const char *path,
+                                         virStorageNetProtocol protocol,
+                                         char **pool,
+                                         char **image)
+{
+
+    g_autofree char *pathcopy = g_strdup(path);
+    char *tmp;
+
+    if (protocol != VIR_STORAGE_NET_PROTOCOL_GLUSTER &&
+        protocol != VIR_STORAGE_NET_PROTOCOL_RBD) {
+
+        if (image)
+            *image = g_steal_pointer(&pathcopy);
+
+        return 0;
+    }
+
+    if (!(tmp = strchr(pathcopy, '/')) || tmp == pathcopy) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("can't split path '%1$s' into pool name and image name"),
+                       pathcopy);
+        return -1;
+    }
+
+    tmp[0] = '\0';
+
+    if (pool)
+        *pool = g_steal_pointer(&pathcopy);
+
+    if (image)
+        *image = g_strdup(tmp + 1);
+
+    return 0;
+}

diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index 48d576e83d1797a93f1d80d133c4017b6b811b9e..17b5d2a99808d521b13c5a79a69a31b9f5f0a745 100644 (file)
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -302,7 +302,6 @@ struct _virStorageSource {
     char *path;
     char *fdgroup; /* name of group of file descriptors the user wishes to use instead of 'path' */
     virStorageNetProtocol protocol;
-    char *volume; /* volume name for remote storage */
     char *snapshot; /* for storage systems supporting internal snapshots */
     char *configFile; /* some storage systems use config file as part of
                          the source definition */
@@ -598,3 +597,9 @@ void
 virStorageSourceInitiatorClear(virStorageSourceInitiatorDef *initiator);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageAuthDef, virStorageAuthDefFree);
+
+int
+virStorageSourceNetworkProtocolPathSplit(const char *path,
+                                         virStorageNetProtocol protocol,
+                                         char **pool,
+                                         char **image);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1b9be478e4c75c92656d628a98f6e7c7a99b6b32..b846011f0fc05c3601eda5153d5202fde79253a3 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1186,6 +1186,7 @@ virStorageSourceIsRelative;
 virStorageSourceIsSameLocation;
 virStorageSourceNetCookiesValidate;
 virStorageSourceNetworkAssignDefaultPorts;
+virStorageSourceNetworkProtocolPathSplit;
 virStorageSourceNew;
 virStorageSourceNVMeDefFree;
 virStorageSourcePoolDefFree;

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 36fba4a55572e08bae25d29ff29bef08ece5e111..9d8301169b393e747baf3dfeb66c06fc487f0e0a 100644 (file)
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1096,7 +1096,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSource *src,
             return NULL;
         }
 
-        virBufferStrcat(&buf, "rbd:", src->volume, "/", src->path, NULL);
+        virBufferStrcat(&buf, "rbd:", src->path, NULL);
 
         if (username) {
             virBufferEscape(&buf, '\\', ":", ":id=%s", username);

diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index b83ed45c46da889d8c351f6933d3bd75fb666c50..b2ff0edcf2d9676babca91ba22fa8619bfca0e30 100644 (file)
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -1476,7 +1476,7 @@ xenFormatXLDiskSrcNet(virStorageSource *src)
             return NULL;
         }
 
-        virBufferStrcat(&buf, "rbd:", src->volume, "/", src->path, NULL);
+        virBufferStrcat(&buf, "rbd:", src->path, NULL);
 
         virBufferAddLit(&buf, ":auth_supported=none");
 

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index a59c0a0a0337084fec5b5cf3cec9c7d41929b6ad..9b370d4c7cce5767e4ebd72fbf6e8e3defb4bec5 100644 (file)
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -213,13 +213,9 @@ qemuBlockStorageSourceGetURI(virStorageSource *src)
     }
 
     if (src->path) {
-        if (src->volume) {
-            uri->path = g_strdup_printf("/%s/%s", src->volume, src->path);
-        } else {
-            uri->path = g_strdup_printf("%s%s",
-                                        g_path_is_absolute(src->path) ? "" : "/",
-                                        src->path);
-        }
+        uri->path = g_strdup_printf("%s%s",
+                                    g_path_is_absolute(src->path) ? "" : "/",
+                                    src->path);
     }
 
     uri->query = g_strdup(src->query);
@@ -401,10 +397,17 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSource *src,
 {
     g_autoptr(virJSONValue) servers = NULL;
     g_autoptr(virJSONValue) props = NULL;
+    g_autofree char *volume = NULL;
+    g_autofree char *path = NULL;
 
     if (!(servers = qemuBlockStorageSourceBuildHostsJSONSocketAddress(src)))
         return NULL;
 
+    if (virStorageSourceNetworkProtocolPathSplit(src->path,
+                                                 VIR_STORAGE_NET_PROTOCOL_GLUSTER,
+                                                 &volume, &path) < 0)
+        return NULL;
+
      /* { driver:"gluster",
       *   volume:"testvol",
       *   path:"/a.img",
@@ -412,8 +415,8 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSource *src,
       *            {type:"unix", socket:"/tmp/glusterd.socket"}, ...]}
       */
     if (virJSONValueObjectAdd(&props,
-                              "s:volume", src->volume,
-                              "s:path", src->path,
+                              "s:volume", volume,
+                              "s:path", path,
                               "a:server", &servers, NULL) < 0)
         return NULL;
 
@@ -662,6 +665,13 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src,
     const char *username = NULL;
     g_autoptr(virJSONValue) authmodes = NULL;
     const char *keysecret = NULL;
+    g_autofree char *pool = NULL;
+    g_autofree char *image = NULL;
+
+    if (virStorageSourceNetworkProtocolPathSplit(src->path,
+                                                 VIR_STORAGE_NET_PROTOCOL_RBD,
+                                                 &pool, &image) < 0)
+        return NULL;
 
     if (src->nhosts > 0 &&
         !(servers = qemuBlockStorageSourceBuildHostsJSONInetSocketAddress(src)))
@@ -715,8 +725,8 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src,
     }
 
     if (virJSONValueObjectAdd(&ret,
-                              "s:pool", src->volume,
-                              "s:image", src->path,
+                              "s:pool", pool,
+                              "s:image", image,
                               "S:snapshot", src->snapshot,
                               "S:conf", src->configFile,
                               "A:server", &servers,

diff --git a/src/storage_file/storage_file_backend_gluster.c b/src/storage_file/storage_file_backend_gluster.c
index abb1c473097f5c8bbea926faea2207b6deab6b9c..8778995b6cdc1f06cd1cdd6d92969742e6bde6f8 100644 (file)
--- a/src/storage_file/storage_file_backend_gluster.c
+++ b/src/storage_file/storage_file_backend_gluster.c
@@ -37,6 +37,7 @@ VIR_LOG_INIT("storage.storage_file_gluster");
 typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv;
 struct _virStorageFileBackendGlusterPriv {
     glfs_t *vol;
+    char *image;
 };
 
 static void
@@ -45,12 +46,13 @@ virStorageFileBackendGlusterDeinit(virStorageSource *src)
     virStorageDriverData *drv = src->drv;
     virStorageFileBackendGlusterPriv *priv = drv->priv;
 
-    VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%u/%s%s)",
-              src, src->hosts->name, src->hosts->port, src->volume, src->path);
+    VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%u/%s)",
+              src, src->hosts->name, src->hosts->port, src->path);
 
     if (priv->vol)
         glfs_fini(priv->vol);
 
+    VIR_FREE(priv->image);
     VIR_FREE(priv);
     drv->priv = NULL;
 }
@@ -98,25 +100,25 @@ virStorageFileBackendGlusterInit(virStorageSource *src)
 {
     virStorageDriverData *drv = src->drv;
     g_autofree virStorageFileBackendGlusterPriv *priv = NULL;
+    g_autofree char *volume = NULL;
+    g_autofree char *image = NULL;
     size_t i;
 
-    if (!src->volume) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("missing gluster volume name for path '%1$s'"),
-                       src->path);
+    if (virStorageSourceNetworkProtocolPathSplit(src->path,
+                                                 VIR_STORAGE_NET_PROTOCOL_GLUSTER,
+                                                 &volume, &image) < 0)
         return -1;
-    }
 
     priv = g_new0(virStorageFileBackendGlusterPriv, 1);
 
     VIR_DEBUG("initializing gluster storage file %p "
               "(priv='%p' volume='%s' path='%s') as [%u:%u]",
-              src, priv, src->volume, src->path,
+              src, priv, volume, image,
               (unsigned int)drv->uid, (unsigned int)drv->gid);
 
-    if (!(priv->vol = glfs_new(src->volume))) {
+    if (!(priv->vol = glfs_new(volume))) {
         virReportError(VIR_ERR_OPERATION_FAILED,
-                       _("failed to create glfs object for '%1$s'"), src->volume);
+                       _("failed to create glfs object for '%1$s'"), volume);
         return -1;
     }
 
@@ -135,6 +137,7 @@ virStorageFileBackendGlusterInit(virStorageSource *src)
         return -1;
     }
 
+    priv->image = g_steal_pointer(&image);
     drv->priv = g_steal_pointer(&priv);
 
     return 0;
@@ -148,7 +151,7 @@ virStorageFileBackendGlusterCreate(virStorageSource *src)
     virStorageFileBackendGlusterPriv *priv = drv->priv;
     glfs_fd_t *fd = NULL;
 
-    if (!(fd = glfs_creat(priv->vol, src->path,
+    if (!(fd = glfs_creat(priv->vol, priv->image,
                           O_CREAT | O_TRUNC | O_WRONLY, S_IRUSR | S_IWUSR)))
         return -1;
 
@@ -163,7 +166,7 @@ virStorageFileBackendGlusterUnlink(virStorageSource *src)
     virStorageDriverData *drv = src->drv;
     virStorageFileBackendGlusterPriv *priv = drv->priv;
 
-    return glfs_unlink(priv->vol, src->path);
+    return glfs_unlink(priv->vol, priv->image);
 }
 
 
@@ -174,7 +177,7 @@ virStorageFileBackendGlusterStat(virStorageSource *src,
     virStorageDriverData *drv = src->drv;
     virStorageFileBackendGlusterPriv *priv = drv->priv;
 
-    return glfs_stat(priv->vol, src->path, st);
+    return glfs_stat(priv->vol, priv->image, st);
 }
 
 
@@ -193,15 +196,15 @@ virStorageFileBackendGlusterRead(virStorageSource *src,
 
     *buf = NULL;
 
-    if (!(fd = glfs_open(priv->vol, src->path, O_RDONLY))) {
+    if (!(fd = glfs_open(priv->vol, priv->image, O_RDONLY))) {
         virReportSystemError(errno, _("Failed to open file '%1$s'"),
-                             src->path);
+                             priv->image);
         return -1;
     }
 
     if (offset > 0) {
         if (glfs_lseek(fd, offset, SEEK_SET) == (off_t) -1) {
-            virReportSystemError(errno, _("cannot seek into '%1$s'"), src->path);
+            virReportSystemError(errno, _("cannot seek into '%1$s'"), priv->image);
             goto cleanup;
         }
     }
@@ -216,7 +219,7 @@ virStorageFileBackendGlusterRead(virStorageSource *src,
             continue;
         if (r < 0) {
             VIR_FREE(*buf);
-            virReportSystemError(errno, _("unable to read '%1$s'"), src->path);
+            virReportSystemError(errno, _("unable to read '%1$s'"), priv->image);
             return r;
         }
         if (r == 0)
@@ -243,7 +246,7 @@ virStorageFileBackendGlusterAccess(virStorageSource *src,
     virStorageDriverData *drv = src->drv;
     virStorageFileBackendGlusterPriv *priv = drv->priv;
 
-    return glfs_access(priv->vol, src->path, mode);
+    return glfs_access(priv->vol, priv->image, mode);
 }
 
 static int
@@ -254,7 +257,7 @@ virStorageFileBackendGlusterChown(const virStorageSource *src,
     virStorageDriverData *drv = src->drv;
     virStorageFileBackendGlusterPriv *priv = drv->priv;
 
-    return glfs_chown(priv->vol, src->path, uid, gid);
+    return glfs_chown(priv->vol, priv->image, uid, gid);
 }
 
 

diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c
index fa59949cf2937846e5c3f320065674d677d4a5ec..843910a0d833813255651e5cd14d06d5a5b7337a 100644 (file)
--- a/src/storage_file/storage_source.c
+++ b/src/storage_file/storage_source.c
@@ -392,8 +392,6 @@ virStorageSourceNewFromBackingRelative(virStorageSource *parent,
 
             def->nhosts = parent->nhosts;
         }
-
-        def->volume = g_strdup(parent->volume);
     } else {
         /* set the type to _FILE, the caller shall update it to the actual type */
         def->type = VIR_STORAGE_TYPE_FILE;

diff --git a/src/storage_file/storage_source_backingstore.c b/src/storage_file/storage_source_backingstore.c
index 4a273fe46ca4452e7fc7506efdecf82b5e549773..700a2f5dcb97ed5dd5fa6c63e1466cf4482ab4a1 100644 (file)
--- a/src/storage_file/storage_source_backingstore.c
+++ b/src/storage_file/storage_source_backingstore.c
@@ -108,27 +108,9 @@ virStorageSourceParseBackingURI(virStorageSource *src,
     src->path = g_strdup(path);
 
     if (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
-        char *tmp;
-
-        if (!src->path) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("missing volume name and path for gluster volume"));
-            return -1;
-        }
-
-        if (!(tmp = strchr(src->path, '/')) ||
-            tmp == src->path) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("missing volume name or file name in gluster source path '%1$s'"),
-                           src->path);
+        if (virStorageSourceNetworkProtocolPathSplit(src->path, src->protocol,
+                                                     NULL, NULL) < 0)
             return -1;
-        }
-
-        src->volume = src->path;
-
-        src->path = g_strdup(tmp + 1);
-
-        tmp[0] = '\0';
     }
 
     src->hosts->port = uri->port;
@@ -211,13 +193,6 @@ virStorageSourceParseRBDColonString(const char *rbdstr,
         *p = '\0';
     }
 
-    /* pool vs. image name */
-    if ((p = strchr(src->path, '/'))) {
-        src->volume = g_steal_pointer(&src->path);
-        src->path = g_strdup(p + 1);
-        *p = '\0';
-    }
-
     /* options */
     if (!options)
         return 0; /* all done */
@@ -703,8 +678,7 @@ virStorageSourceParseBackingJSONGluster(virStorageSource *src,
     src->type = VIR_STORAGE_TYPE_NETWORK;
     src->protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER;
 
-    src->volume = g_strdup(volume);
-    src->path = g_strdup(path);
+    src->path = g_strdup_printf("%s/%s", volume, path);
 
     nservers = virJSONValueArraySize(server);
     if (nservers == 0) {
@@ -959,8 +933,7 @@ virStorageSourceParseBackingJSONRBD(virStorageSource *src,
         return -1;
     }
 
-    src->volume = g_strdup(pool);
-    src->path = g_strdup(image);
+    src->path = g_strdup_printf("%s/%s", pool, image);
     src->snapshot = g_strdup(snapshot);
     src->configFile = g_strdup(conf);