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

[Peter Krempa]

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 at outscale.com>
> ---
>  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
-------------- 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/20150512/23569209/attachment-0001.sig>