Re: [libvirt] [PATCH v6 08/13] domain_conf: Read and Write quorum config

[Peter Krempa]

On Thu, Oct 29, 2015 at 14:43:15 +0100, Matthias Gatto wrote:
> Add the capabiltty to libvirt to parse and format the quorum syntax
> as described here:
> http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html
> 
> As explain Peter Krempa in the V5, we need a different index for each child to
> manipulate individually each child of a quorum,
> this tack is done in this patch.
> 
> Sadly this versions is a little buggy: if one day we allow a quorum child
> to have a backing store, we would be unable to made a difference
> between a quorum child and a backing store, worst than that,
> if we have a quorum, with a quorum as a child, we have no way to
> use the index for quorum child manipulation.

Erm, this can happen already today if you use qcow as backing for the
quorum, so we'll need to take care of it right away. Additionally in
such case the code needs to be able to traverse the backing chain for
every single image backing the quorum so that they can be labelled later
on.

The subsequent backing store elements NEED to be uniquely numbered/named
in any case.

> 
> For now, this serie of patch forbid all actions which need
> to use indexes with quorum.

It actually doesn't forbid all of them.

> Therefore even if the index manipulation is buggy, this should not be a 
> problem
> because the buggy code should never be call.

I'm afraid that by not doing this properly right away this will just
lead to more and more problems.

> 
> Signed-off-by: Matthias Gatto 
> ---
>  src/conf/domain_conf.c | 178 
> ++---
>  1 file changed, 126 insertions(+), 52 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ce8edef..363ef5d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6607,20 +6607,56 @@ virDomainDiskSourceParse(xmlNodePtr node,
>  }
>  
>  
> +static bool

This reports libvirt error so you should return -1/0 instead of bool.

> +virDomainDiskThresholdParse(virStorageSourcePtr src,
> +xmlNodePtr node)
> +{
> +char *threshold = virXMLPropString(node, "threshold");

I'd suggest you use virXPathString.

> +int ret;
> +
> +if (!threshold) {
> +virReportError(VIR_ERR_XML_ERROR,
> +   "%s", _("missing threshold in quorum"));
> +return false;
> +}
> +ret = virStrToLong_ul(threshold, NULL, 10, >threshold);
> +if (ret < 0 || src->threshold < 2) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unexpected threshold %s"),
> +   "threshold must be a decimal number superior to 2 "

greater than

> +   "and inferior to the number of children");

less than

> +VIR_FREE(threshold);
> +return false;
> +}
> +VIR_FREE(threshold);
> +return true;
> +}
> +
> +
>  static int
>  virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
> -   virStorageSourcePtr src)
> +   virStorageSourcePtr src,
> +   xmlNodePtr node,
> +   size_t pos)
>  {
>  virStorageSourcePtr backingStore = NULL;
>  xmlNodePtr save_ctxt = ctxt->node;
> -xmlNodePtr source;
> +xmlNodePtr source = NULL;
>  char *type = NULL;
>  char *format = NULL;
>  int ret = -1;
>  
> -if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
> -ret = 0;
> -goto cleanup;
> +if (virStorageSourceIsRAID(src)) {
> +if (!node) {
> +ret = 0;
> +goto cleanup;
> +}
> +ctxt->node = node;
> +} else {
> +if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
> +ret = 0;
> +goto cleanup;

This looks weird. Why are there two cases? The function should really
parse anything regardless whether it's a quorum backing node or not.

The function might need to be turned around so that it actually returns
the parsed backing store rather than directly setting it though.

> +}
>  }
>  
>  if (VIR_ALLOC(backingStore) < 0)
> @@ -6652,17 +6688,36 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr 
> ctxt,
>  goto cleanup;
>  }
>  
> -if (!(source = virXPathNode("./source", ctxt))) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> -   _("missing disk backing store source"));
> -goto cleanup;
> -}
> +if (virStorageSourceIsRAID(backingStore)) {
> +xmlNodePtr cur = NULL;
>  
> -if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 ||
> -virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
> -goto cleanup;
> +if (!virDomainDiskThresholdParse(backingStore, node))
> +goto cleanup;
>  
> -if (!virStorageSourceSetBackingStore(src, backingStore, 0))
> +for (cur = node->children; cur != 

[libvirt] [PATCH v6 08/13] domain_conf: Read and Write quorum config

[Matthias Gatto]

Add the capabiltty to libvirt to parse and format the quorum syntax
as described here:
http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html

As explain Peter Krempa in the V5, we need a different index for each child to
manipulate individually each child of a quorum,
this tack is done in this patch.

Sadly this versions is a little buggy: if one day we allow a quorum child
to have a backing store, we would be unable to made a difference
between a quorum child and a backing store, worst than that,
if we have a quorum, with a quorum as a child, we have no way to
use the index for quorum child manipulation.

For now, this serie of patch forbid all actions which need
to use indexes with quorum.
Therefore even if the index manipulation is buggy, this should not be a problem
because the buggy code should never be call.

Signed-off-by: Matthias Gatto 
---
 src/conf/domain_conf.c | 178 ++---
 1 file changed, 126 insertions(+), 52 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ce8edef..363ef5d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6607,20 +6607,56 @@ virDomainDiskSourceParse(xmlNodePtr node,
 }
 
 
+static bool
+virDomainDiskThresholdParse(virStorageSourcePtr src,
+xmlNodePtr node)
+{
+char *threshold = virXMLPropString(node, "threshold");
+int ret;
+
+if (!threshold) {
+virReportError(VIR_ERR_XML_ERROR,
+   "%s", _("missing threshold in quorum"));
+return false;
+}
+ret = virStrToLong_ul(threshold, NULL, 10, >threshold);
+if (ret < 0 || src->threshold < 2) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unexpected threshold %s"),
+   "threshold must be a decimal number superior to 2 "
+   "and inferior to the number of children");
+VIR_FREE(threshold);
+return false;
+}
+VIR_FREE(threshold);
+return true;
+}
+
+
 static int
 virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
-   virStorageSourcePtr src)
+   virStorageSourcePtr src,
+   xmlNodePtr node,
+   size_t pos)
 {
 virStorageSourcePtr backingStore = NULL;
 xmlNodePtr save_ctxt = ctxt->node;
-xmlNodePtr source;
+xmlNodePtr source = NULL;
 char *type = NULL;
 char *format = NULL;
 int ret = -1;
 
-if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
-ret = 0;
-goto cleanup;
+if (virStorageSourceIsRAID(src)) {
+if (!node) {
+ret = 0;
+goto cleanup;
+}
+ctxt->node = node;
+} else {
+if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
+ret = 0;
+goto cleanup;
+}
 }
 
 if (VIR_ALLOC(backingStore) < 0)
@@ -6652,17 +6688,36 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
 goto cleanup;
 }
 
-if (!(source = virXPathNode("./source", ctxt))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing disk backing store source"));
-goto cleanup;
-}
+if (virStorageSourceIsRAID(backingStore)) {
+xmlNodePtr cur = NULL;
 
-if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 ||
-virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
-goto cleanup;
+if (!virDomainDiskThresholdParse(backingStore, node))
+goto cleanup;
 
-if (!virStorageSourceSetBackingStore(src, backingStore, 0))
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) {
+if ((virDomainDiskBackingStoreParse(ctxt,
+backingStore,
+cur,
+
backingStore->nBackingStores) < 0)) {
+goto cleanup;
+}
+}
+}
+} else {
+
+if (!(source = virXPathNode("./source", ctxt))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing disk backing store source"));
+goto cleanup;
+}
+
+if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 ||
+virDomainDiskBackingStoreParse(ctxt, backingStore, NULL, 0) < 0)
+goto cleanup;
+}
+
+if (!virStorageSourceSetBackingStore(src, backingStore, pos))
 goto cleanup;
 ret = 0;
 
@@ -7177,6 +7232,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) {
 /* boot is parsed as part of virDomainDeviceInfoParseXML */
+