Re: [libvirt] [PATCH v5 8/9] qemu: Add quorum support in qemuBuildDriveDevStr

On Thu, Apr 23, 2015 at 14:41:20 +0200, Matthias Gatto wrote: > Allow to libvirt to build the quorum string used by quemu. > > Add 2 static functions: qemuBuildQuorumStr and > qemuBuildAndAppendDriveStrToVirBuffer. > qemuBuildQuorumStr is made because a quorum can have another quorum > as a child, so we may need to call qemuBuildQuorumStr recursively. > > qemuBuildQuorumFileSourceStr was basically made to share > the code use to build the source between qemuBuildQuorumStr and > qemuBuildDriveStr, but there is some difference betwin the syntax > use by libvirt to declare a disk and the one qemu need to build a quorum: > a quorum need a syntaxe like: > "domaine_name.children.X.file.filename=filename" > where libvirt don't use "file.filename=" but directly "file=". > Therfore I use this function only for quorum. > > But as explained in the cover letter and here: > http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html > We miss some informations in _virStorageSource to have a complet > quorum support in libvirt. > Ideally I'd like to refactore virDomainDiskDefFormat to allow > qemuBuildQuorumStr to call this function in a loop. > > Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com&gt; > --- > src/qemu/qemu_command.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 110 insertions(+) > ... > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 6c40d3e..80cbb7d 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3479,6 +3479,111 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) > return -1; > } > > +static bool > +qemuBuildQuorumFileSourceStr(virConnectPtr conn, > + virStorageSourcePtr src, > + virBuffer *opt, > + const char *toAppend) > +{ > + char *source = NULL; > + int actualType = virStorageSourceGetActualType(src); > + > + if (qemuGetDriveSourceString(src, conn, &source) < 0) > + goto error; > + > + if (source) { > + > + virBufferAsprintf(opt, ",%sfilename=", toAppend); virBufferStrcat > + > + switch (actualType) { > + case VIR_STORAGE_TYPE_DIR: I'd forbid the DIR type altogether with quorums. > + /* QEMU only supports magic FAT format for now */ > + if (src->format > 0 && > + src->format != VIR_STORAGE_FILE_FAT) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unsupported disk driver type for '%s'"), > + virStorageFileFormatTypeToString(src->format)); > + goto error; > + } > + > + if (!src->readonly) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("cannot create virtual FAT disks in read-write mode")); > + goto error; > + } > + > + virBufferAddLit(opt, "fat:"); > + > + break; > + > + default: > + break; > + } > + virBufferAsprintf(opt, "%s", source); virBufferAdd > + } > + > + return true; > + error: > + return false; Error can be returned right away since there is nothing to clean up. > +} > + > + > +static bool > +qemuBuildQuorumStr(virConnectPtr conn, > + virDomainDiskDefPtr disk, > + virStorageSourcePtr src, > + virBuffer *opt, > + const char *toAppend) > +{ > + char *tmp = NULL; > + int ret; > + virStorageSourcePtr backingStore; > + size_t i; > + > + if (!src->threshold) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("threshold missing in the quorum configuration")); > + return false; > + } > + if (src->nBackingStores < 2) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("a quorum must have at last 2 children")); > + return false; > + } > + if (src->threshold > src->nBackingStores) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("threshold must not exceed the number of childrens")); 'children' is the proper plural > + return false; > + } > + virBufferAsprintf(opt, ",%svote-threshold=%lu", > + toAppend, src->threshold); > + for (i = 0; i < src->nBackingStores; ++i) { > + backingStore = virStorageSourceGetBackingStore(src, i); > + ret = virAsprintf(&tmp, "%schildren.%lu.file.", toAppend, i); > + if (ret < 0) > + return false; > + > + virBufferAsprintf(opt, ",%schildren.%lu.driver=%s", > + toAppend, i, > + virStorageFileFormatTypeToString(backingStore->format)); > + > + if (qemuBuildQuorumFileSourceStr(conn, backingStore, opt, tmp) == false) > + goto error; > + > + /* This operation avoid me to made another copy */ > + tmp[ret - sizeof("file")] = '\0'; > + if (backingStore->type == VIR_STORAGE_TYPE_QUORUM) { > + if (!qemuBuildQuorumStr(conn, disk, backingStore, opt, tmp)) > + goto error; This won't work if the quorum would be after an intermediate layer. That's why this needs the internal backing chain tracking. > + } > + VIR_FREE(tmp); > + } > + return true; > + error: > + VIR_FREE(tmp); > + return false; > +} > + > > /* Qemu 1.2 and later have a binary flag -enable-fips that must be > * used for VNC auth to obey FIPS settings; but the flag only > @@ -3952,6 +4057,11 @@ qemuBuildDriveStr(virConnectPtr conn, > disk->blkdeviotune.size_iops_sec); > } > > + if (actualType == VIR_STORAGE_TYPE_QUORUM) { > + if (!qemuBuildQuorumStr(conn, disk, disk->src, &opt, "")) > + goto error; > + } This is rather ugly. Since qemuBuildDriveStr skips the quorum storage sources rather accidentally as they appear "empty" to the virStorageSourceIsEmpty function. If you want/need to have a different function you should make it explicit how the code is formatting the source. Additionally once the quorum disk won't be on top, sice you for example created a snapshot on top of the quorum unified device, then this code will terribly break since qemuBuildQuorumStr will never be called. As I've explained in a previous patch, quorums will need libvirt to do the houskeeping of the whole backing chain internally since it cannot be recreated that easily. Peter