[libvirt] [PATCH v6 09/13] qemu: Add quorum support in qemuBuildDriveDevStr

[Peter Krempa]

On Thu, Oct 29, 2015 at 14:43:16 +0100, Matthias Gatto wrote:
> Allow libvirt to build the quorum string used by quemu.
> 
> Add 2 static functions: qemuBuildRAIDStr and
> qemuBuildRAIDFileSourceStr.
> qemuBuildRAIDStr is made because a quorum can have another quorum
> as a child, so we may need to call qemuBuildRAIDStr recursively.
> 
> qemuBuildRAIDFileSourceStr was basically made to share
> the code use to build the source between qemuBuildRAIDStr 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.
> 
> Signed-off-by: Matthias Gatto <matthias.gatto at outscale.com>
> ---
>  src/qemu/qemu_command.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 50cf8cc..4ca0011 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3612,6 +3612,93 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
>      return -1;
>  }
>  
> +static bool

The same comment regarding return value as in previous cases.

> +qemuBuildRAIDFileSourceStr(virConnectPtr conn,
> +                           virStorageSourcePtr src,
> +                           virBuffer *opt,
> +                           const char *toAppend)

Since 'toAppend' is always pre-pended I'd rather call it prefix.

> +{
> +    char *source = NULL;
> +    int actualType = virStorageSourceGetActualType(src);
> +
> +    if (qemuGetDriveSourceString(src, conn, &source) < 0)

Are you sure that it will work with remote storage too?

> +        return false;
> +
> +    if (source) {
> +        virBufferStrcat(opt, ",", toAppend, "filename=", NULL);

Since you already have 'source' here you can append it right away ...

> +
> +        if (actualType == VIR_STORAGE_TYPE_DIR) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("unsupported disk driver type for '%s'"),
> +                           virStorageFileFormatTypeToString(src->format));

This should probably be extracted somewhere else so that there's a
single point where we can check it.

> +            return false;
> +        }
> +        virBufferAdd(opt, source, -1);

... rather than here.

> +    }
> +
> +    return true;
> +}
> +
> +
> +static bool
> +qemuBuildRAIDStr(virConnectPtr conn,
> +                 virDomainDiskDefPtr disk,
> +                 virStorageSourcePtr src,
> +                 virBuffer *opt,
> +                 const char *toAppend)
> +{
> +    char *tmp = NULL;
> +    int ret;
> +    virStorageSourcePtr backingStore;
> +    size_t i;
> +
> +    if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_QUORUM) {
> +        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 children"));
> +            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);

So here you create 'tmp' ...

> +        if (ret < 0)
> +            return false;
> +
> +        virBufferAsprintf(opt, ",%schildren.%lu.driver=%s",
> +                          toAppend, i,
> +                          virStorageFileFormatTypeToString(backingStore->format));
> +
> +        if (qemuBuildRAIDFileSourceStr(conn, backingStore, opt, tmp) == false)

.. this function reads it ... 

> +            goto error;
> +
> +        /* This operation avoid us to made another copy */
> +        tmp[ret - sizeof("file")] = '\0';

... so why is this necessary? Also I think it has a off-by-one which is
transparet since the string is containing a trailing dot, and sizeof
returns the size including the 0-byte at the end of the string.


> +        if (virStorageSourceIsRAID(backingStore)) {
> +            if (!qemuBuildRAIDStr(conn, disk, backingStore, opt, tmp))
> +                goto error;
> +        }
> +        VIR_FREE(tmp);
> +    }
> +    return true;
> + error:
> +    VIR_FREE(tmp);
> +    return false;
> +}
> +
>  
>  /* Check whether the device address is using either 'ccw' or default s390
>   * address format and whether that's "legal" for the current qemu and/or

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/9d83b8a3/attachment-0001.sig>