Re: [libvirt] [PATCH v5 7/9] domain_conf: Read and Write quorum config

On Thu, Apr 23, 2015 at 14:41:19 +0200, 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 > > Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com&gt; > --- > src/conf/domain_conf.c | 164 +++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 119 insertions(+), 45 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index a3a6c13..ec93b6f 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5952,20 +5952,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, &src->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 (src->type == VIR_STORAGE_TYPE_QUORUM) { > + 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) > @@ -5997,6 +6033,25 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, > goto cleanup; > } > > + if (backingStore->type == VIR_STORAGE_TYPE_QUORUM) { > + xmlNodePtr cur = NULL; > + > + if (!virDomainDiskThresholdParse(backingStore, node)) > + goto cleanup; > + > + 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; > + } > + } > + } > + goto exit; > + } > + > if (!(source = virXPathNode("./source", ctxt))) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("missing disk backing store source")); > @@ -6004,10 +6059,11 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, > } > > if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || > - virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) > + virDomainDiskBackingStoreParse(ctxt, backingStore, NULL, 0) < 0) > goto cleanup; > > - if (!virStorageSourceSetBackingStore(src, backingStore, 0)) > + exit: > + if (!virStorageSourceSetBackingStore(src, backingStore, pos)) > goto cleanup; > ret = 0; > > @@ -6020,7 +6076,6 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, > return ret; > } > > - > #define VENDOR_LEN 8 > #define PRODUCT_LEN 16 > > @@ -6518,6 +6573,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, > } > } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) { > /* boot is parsed as part of virDomainDeviceInfoParseXML */ > + } else if (xmlStrEqual(cur->name, BAD_CAST "backingStore")) { > + if (virDomainDiskBackingStoreParse(ctxt, def->src, cur, > + def->src->nBackingStores) < 0) > + goto error; > } > } > cur = cur->next; > @@ -6541,12 +6600,19 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, > def->device = VIR_DOMAIN_DISK_DEVICE_DISK; > } > > + if (def->src->type == VIR_STORAGE_TYPE_QUORUM && > + !virDomainDiskThresholdParse(def->src, node)) > + goto error; > + > + snapshot = virXMLPropString(node, "snapshot"); > + > /* Only CDROM and Floppy devices are allowed missing source path > * to indicate no media present. LUN is for raw access CD-ROMs > * that are not attached to a physical device presently */ > if (virStorageSourceIsEmpty(def->src) && > (def->device == VIR_DOMAIN_DISK_DEVICE_DISK || > - (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE))) { > + (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) && > + def->src->type != VIR_STORAGE_TYPE_QUORUM) { > virReportError(VIR_ERR_NO_SOURCE, > target ? "%s" : NULL, target); > goto error; > @@ -6892,9 +6958,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, > if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE > && virDomainDiskDefAssignAddress(xmlopt, def) < 0) > goto error; > - > - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) > - goto error; > } > > cleanup: > @@ -17717,12 +17780,14 @@ virDomainDiskSourceFormat(virBufferPtr buf, > > static int > virDomainDiskBackingStoreFormat(virBufferPtr buf, > - virStorageSourcePtr backingStore, > - const char *backingStoreRaw, > + virStorageSourcePtr src, > unsigned int idx) > { > + const char *backingStoreRaw = src->backingStoreRaw; > + virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(src, 0); > const char *type; > const char *format; > + size_t i = 0; > > if (!backingStore) { > if (!backingStoreRaw) > @@ -17730,37 +17795,43 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, > return 0; > } > > - if (!backingStore->type || > - !(type = virStorageTypeToString(backingStore->type))) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("unexpected disk backing store type %d"), > - backingStore->type); > - return -1; > - } > + do { > + backingStore = virStorageSourceGetBackingStore(src, i); > + if (!backingStore->type || > + !(type = virStorageTypeToString(backingStore->type))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unexpected disk backing store type %d"), > + backingStore->type); > + return -1; > + } > > - if (backingStore->format <= 0 || > - !(format = virStorageFileFormatTypeToString(backingStore->format))) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("unexpected disk backing store format %d"), > - backingStore->format); > - return -1; > - } > + if (backingStore->format <= 0 || > + !(format = virStorageFileFormatTypeToString(backingStore->format))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unexpected disk backing store format %d"), > + backingStore->format); > + return -1; > + } > > - virBufferAsprintf(buf, "<backingStore type='%s' index='%u'>\n", > - type, idx); > - virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<backingStore type='%s' index='%u'", > + type, idx); So now this change is wrong. Every single backing store element of the driver will have the same index, so you will not be able to reference them uniquely later. The indexes were designed in such way that they can be used to address backing chain elements (as node names in qemu) so you cannot make them pointless. Note that there's a lot of functions that use the information especially in the block job code that will need to be fixed. Additionally you'll need to fix the block job code in such way that it will work correctly with the quorums. Also note that a lot of places in the code don't properly implement transformations of the backing chain after block operations finish and resort to re-detection of the backing chain. That obviously won't work with quorums since they are not exposed in the backing chain (afaik). This means that basically for a proper implementation of this you'll need either to fix all code that alters the backing chains so that it works properly and then implement quorums on top of that or block basically every operation that transforms the backing chain so that the internal state does not get corrupted. Additionally this will probably also require modifying the storage handling code in libvirt in such way that it will handle the backing store information internally and not try to reload the state from the metadata in the image. Peter