[libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function

Peter Krempa posted 5 patches 8 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function
Posted by Peter Krempa 8 years, 10 months ago
To allow testing of the algorithm, split out the extractor into a
separate helper.
---
 src/storage/storage_util.c | 95 +++++++++++++++++++++++++++-------------------
 src/storage/storage_util.h |  4 ++
 2 files changed, 59 insertions(+), 40 deletions(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index cad706199..3b55bafc0 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -2836,6 +2836,60 @@ virStorageBackendDeleteLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
 }


+int
+virStorageUtilGlusterExtractPoolSources(const char *host,
+                                        const char *xml,
+                                        virStoragePoolSourceListPtr list,
+                                        bool netfs)
+{
+    xmlDocPtr doc = NULL;
+    xmlXPathContextPtr ctxt = NULL;
+    xmlNodePtr *nodes = NULL;
+    virStoragePoolSource *src = NULL;
+    size_t i;
+    int nnodes;
+    int ret = -1;
+
+    if (!(doc = virXMLParseStringCtxt(xml, _("(gluster_cli_output)"), &ctxt)))
+        goto cleanup;
+
+    if ((nnodes = virXPathNodeSet("//volumes/volume", ctxt, &nodes)) < 0)
+        goto cleanup;
+
+    for (i = 0; i < nnodes; i++) {
+        ctxt->node = nodes[i];
+
+        if (!(src = virStoragePoolSourceListNewSource(list)))
+            goto cleanup;
+
+        if (!(src->dir = virXPathString("string(//name)", ctxt))) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("failed to extract gluster volume name"));
+            goto cleanup;
+        }
+
+        if (VIR_ALLOC_N(src->hosts, 1) < 0)
+            goto cleanup;
+        src->nhost = 1;
+
+        if (VIR_STRDUP(src->hosts[0].name, host) < 0)
+            goto cleanup;
+
+        if (netfs)
+            src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS;
+    }
+
+    ret = nnodes;
+
+ cleanup:
+    VIR_FREE(nodes);
+    xmlXPathFreeContext(ctxt);
+    xmlFreeDoc(doc);
+
+    return ret;
+}
+
+
 /**
  * virStorageBackendFindGlusterPoolSources:
  * @host: host to detect volumes on
@@ -2859,12 +2913,6 @@ virStorageBackendFindGlusterPoolSources(const char *host,
     char *glusterpath = NULL;
     char *outbuf = NULL;
     virCommandPtr cmd = NULL;
-    xmlDocPtr doc = NULL;
-    xmlXPathContextPtr ctxt = NULL;
-    xmlNodePtr *nodes = NULL;
-    virStoragePoolSource *src = NULL;
-    size_t i;
-    int nnodes;
     int rc;

     int ret = -1;
@@ -2895,42 +2943,9 @@ virStorageBackendFindGlusterPoolSources(const char *host,
         goto cleanup;
     }

-    if (!(doc = virXMLParseStringCtxt(outbuf, _("(gluster_cli_output)"),
-                                      &ctxt)))
-        goto cleanup;
-
-    if ((nnodes = virXPathNodeSet("//volumes/volume", ctxt, &nodes)) < 0)
-        goto cleanup;
-
-    for (i = 0; i < nnodes; i++) {
-        ctxt->node = nodes[i];
-
-        if (!(src = virStoragePoolSourceListNewSource(list)))
-            goto cleanup;
-
-        if (!(src->dir = virXPathString("string(//name)", ctxt))) {
-            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("failed to extract gluster volume name"));
-            goto cleanup;
-        }
-
-        if (VIR_ALLOC_N(src->hosts, 1) < 0)
-            goto cleanup;
-        src->nhost = 1;
-
-        if (VIR_STRDUP(src->hosts[0].name, host) < 0)
-            goto cleanup;
-
-        if (netfs)
-            src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS;
-    }
-
-    ret = nnodes;
+    ret = virStorageUtilGlusterExtractPoolSources(host, outbuf, list, netfs);

  cleanup:
-    VIR_FREE(nodes);
-    xmlXPathFreeContext(ctxt);
-    xmlFreeDoc(doc);
     VIR_FREE(outbuf);
     virCommandFree(cmd);
     VIR_FREE(glusterpath);
diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h
index 741baa78d..ecd67e2fc 100644
--- a/src/storage/storage_util.h
+++ b/src/storage/storage_util.h
@@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn,
 int virStorageBackendRefreshLocal(virConnectPtr conn,
                                   virStoragePoolObjPtr pool);

+int virStorageUtilGlusterExtractPoolSources(const char *host,
+                                            const char *xml,
+                                            virStoragePoolSourceListPtr list,
+                                            bool netfs);
 int virStorageBackendFindGlusterPoolSources(const char *host,
                                             virStoragePoolSourceListPtr list,
                                             bool netfs,
-- 
2.12.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function
Posted by Andrea Bolognani 8 years, 10 months ago
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
[...]
> @@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn,
>  int virStorageBackendRefreshLocal(virConnectPtr conn,
>                                    virStoragePoolObjPtr pool);
> 
> +int virStorageUtilGlusterExtractPoolSources(const char *host,
> +                                            const char *xml,
> +                                            virStoragePoolSourceListPtr list,
> +                                            bool netfs);

Please add a comment along the lines of "For test suite use
only" here. Ideally we'd have a separate *priv.h header file
to be used for the purpose, but that's out of scope.


ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function
Posted by Peter Krempa 8 years, 10 months ago
On Mon, Apr 03, 2017 at 16:20:26 +0200, Andrea Bolognani wrote:
> On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
> [...]
> > @@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn,
> >  int virStorageBackendRefreshLocal(virConnectPtr conn,
> >                                    virStoragePoolObjPtr pool);
> > 
> > +int virStorageUtilGlusterExtractPoolSources(const char *host,
> > +                                            const char *xml,
> > +                                            virStoragePoolSourceListPtr list,
> > +                                            bool netfs);
> 
> Please add a comment along the lines of "For test suite use
> only" here. Ideally we'd have a separate *priv.h header file
> to be used for the purpose, but that's out of scope.

Why? This function can be used wherever it's necessary. It's by no-means
specific to the test suite.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function
Posted by Andrea Bolognani 8 years, 10 months ago
On Mon, 2017-04-03 at 16:38 +0200, Peter Krempa wrote:
> > > +int virStorageUtilGlusterExtractPoolSources(const char *host,
> > > +                                            const char *xml,
> > > +                                            virStoragePoolSourceListPtr list,
> > > +                                            bool netfs);
> > 
> > Please add a comment along the lines of "For test suite use
> > only" here. Ideally we'd have a separate *priv.h header file
> > to be used for the purpose, but that's out of scope.
> 
> Why? This function can be used wherever it's necessary. It's by no-means
> specific to the test suite.

It would probably be a static function, or would never have
been ripped out of virStorageBackendFindGlusterPoolSources()
to begin with, were not for the test suite, so I'd say the
comment would be warranted until a non-theoretical use for
it is found outside of the test suite.

If you don't feel the same way, you can safely disregard my
remark though.

-- 
Andrea Bolognani / Red Hat / Virtualization

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