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@xxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list