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

[Matthias Gatto]

On Tue, May 12, 2015 at 5:38 PM, Peter Krempa <pkrempa at redhat.com> wrote:
> 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

I don't really understand what you mean by:
"quorums will need libvirt to do
the houskeeping of the whole backing chain internally"

Libvirt, already keep information about the backing chain,
do you think that we need function, which simplify the manipulation
of the backing chain, like function allowing to get/set/modify
a backingStore taking in parameter an index
and the virDomainDiskDefPtr of the quorum ?
Do you think that we miss information for the backingStore ?

Matthias