[libvirt] [PATCH v2 20/29] conf: Allow convenient lookup of <source> in virDomainStorageSourceParse

Peter Krempa posted 29 patches 6 years, 10 months ago
[libvirt] [PATCH v2 20/29] conf: Allow convenient lookup of <source> in virDomainStorageSourceParse
Posted by Peter Krempa 6 years, 10 months ago
If NULL is passed, the function will lookup <source> in current context.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/conf/domain_conf.c | 12 +++++++++++-
 src/conf/domain_conf.h |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5773d07474..852489e185 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9080,7 +9080,7 @@ virDomainStorageSourceParseBase(const char *type,

 /**
  * virDomainStorageSourceParse:
- * @node: XML node pointing to the source element to parse
+ * @node: XML node pointing to the source element to parse (see below)
  * @ctxt: XPath context
  * @src: filled with parsed data
  * @flags: XML parser flags
@@ -9089,6 +9089,9 @@ virDomainStorageSourceParseBase(const char *type,
  * Parses @src definition from element pointed to by @node. Note that this
  * does not parse the 'type' and 'format' attributes of @src and 'type' needs
  * to be set correctly prior to calling this function.
+ *
+ * If @node is NULL a <source> subelement is looked up in @ctxt to be used as
+ * source. Error is reported if the source is not found.
  */
 int
 virDomainStorageSourceParse(xmlNodePtr node,
@@ -9100,6 +9103,13 @@ virDomainStorageSourceParse(xmlNodePtr node,
     VIR_XPATH_NODE_AUTORESTORE(ctxt);
     xmlNodePtr tmp;

+    if (!node &&
+        !(node = virXPathNode("./source", ctxt))) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing <source> element for storage source"));
+        return -1;
+    }
+
     ctxt->node = node;

     switch ((virStorageType)src->type) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ce6e5b4748..6fb73bdaf7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3463,7 +3463,7 @@ int virDomainStorageSourceParse(xmlNodePtr node,
                                 virStorageSourcePtr src,
                                 unsigned int flags,
                                 virDomainXMLOptionPtr xmlopt)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);

 int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
                                      int maplen,
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 20/29] conf: Allow convenient lookup of <source> in virDomainStorageSourceParse
Posted by Ján Tomko 6 years, 10 months ago
On Fri, Mar 22, 2019 at 07:00:56PM +0100, Peter Krempa wrote:
>If NULL is passed, the function will lookup <source> in current context.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/conf/domain_conf.c | 12 +++++++++++-
> src/conf/domain_conf.h |  2 +-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 5773d07474..852489e185 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -9080,7 +9080,7 @@ virDomainStorageSourceParseBase(const char *type,
>
> /**
>  * virDomainStorageSourceParse:
>- * @node: XML node pointing to the source element to parse
>+ * @node: XML node pointing to the source element to parse (see below)
>  * @ctxt: XPath context
>  * @src: filled with parsed data
>  * @flags: XML parser flags
>@@ -9089,6 +9089,9 @@ virDomainStorageSourceParseBase(const char *type,
>  * Parses @src definition from element pointed to by @node. Note that this
>  * does not parse the 'type' and 'format' attributes of @src and 'type' needs
>  * to be set correctly prior to calling this function.
>+ *
>+ * If @node is NULL a <source> subelement is looked up in @ctxt to be used as
>+ * source. Error is reported if the source is not found.
>  */
> int
> virDomainStorageSourceParse(xmlNodePtr node,
>@@ -9100,6 +9103,13 @@ virDomainStorageSourceParse(xmlNodePtr node,
>     VIR_XPATH_NODE_AUTORESTORE(ctxt);
>     xmlNodePtr tmp;
>
>+    if (!node &&
>+        !(node = virXPathNode("./source", ctxt))) {
>+        virReportError(VIR_ERR_XML_ERROR, "%s",
>+                       _("missing <source> element for storage source"));
>+        return -1;
>+    }
>+

This feels odd to me and there is only one caller using this at the end
of the series. Most of the callers already have a node handy and there's
the migrationSource vs. source where the 'source' node is preferred and
optional.

I'd rather open-code the lookup in DiskDefMirrorParse

Jano

>     ctxt->node = node;
>
>     switch ((virStorageType)src->type) {
>diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>index ce6e5b4748..6fb73bdaf7 100644
>--- a/src/conf/domain_conf.h
>+++ b/src/conf/domain_conf.h
>@@ -3463,7 +3463,7 @@ int virDomainStorageSourceParse(xmlNodePtr node,
>                                 virStorageSourcePtr src,
>                                 unsigned int flags,
>                                 virDomainXMLOptionPtr xmlopt)
>-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>
> int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
>                                      int maplen,
>-- 
>2.20.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list