[libvirt] [PATCH v6 08/13] domain_conf: Read and Write quorum config
Peter Krempa
pkrempa at redhat.com
Mon Nov 2 12:00:38 UTC 2015
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 <matthias.gatto at outscale.com>
> ---
> 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, &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 "
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 != 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;
Basically all of the above code should parse unconditionally any tree of
backing stores. Later on you can verify that the configuration makes
sense. Ideally in the per-driver post parse callback, so that certain
configs can be enabled or disabled on a per-driver basis.
>
> @@ -7177,6 +7232,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;
> @@ -7204,12 +7263,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");
Why is this necessary? I couldn't find a line where you'd delete it or
use it in any way.
> +
> /* 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)) &&
> + !(virStorageSourceIsRAID(def->src))) {
> virReportError(VIR_ERR_NO_SOURCE,
> target ? "%s" : NULL, target);
> goto error;
> @@ -7552,10 +7618,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> }
>
> - if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) {
> - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0)
> - goto error;
> - }
>
> cleanup:
> VIR_FREE(bus);
> @@ -18863,12 +18925,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)
> @@ -18876,37 +18940,44 @@ 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 ||
This crashes if the returned backingStore will be NULL which could
possibly happen due to the way how virStorageSourceSetBackingStore is
able to create holes in the array.
> + !(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);
> + if (backingStore->threshold)
> + virBufferAsprintf(buf, " threshold='%lu'", backingStore->threshold);
> + virBufferAddLit(buf, ">\n");
> + virBufferAdjustIndent(buf, 2);
>
> - virBufferAsprintf(buf, "<format type='%s'/>\n", format);
> - /* We currently don't output seclabels for backing chain element */
> - if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
> - virDomainDiskBackingStoreFormat(buf,
> - virStorageSourceGetBackingStore(backingStore, 0),
> - backingStore->backingStoreRaw,
> - idx + 1) < 0)
> - return -1;
> + virBufferAsprintf(buf, "<format type='%s'/>\n", format);
> + /* We currently don't output seclabels for backing chain element */
> + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
> + virDomainDiskBackingStoreFormat(buf,
> + backingStore,
> + idx + 1) < 0)
> + return -1;
>
> - virBufferAdjustIndent(buf, -2);
> - virBufferAddLit(buf, "</backingStore>\n");
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</backingStore>\n");
> + ++i;
> + ++idx;
> + } while (i < src->nBackingStores);
Please use a for loop.
> return 0;
> }
>
Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151102/e5cf4c1d/attachment-0001.sig>
More information about the libvir-list
mailing list