Re: [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 
> ---
>  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, ) < 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(, "%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 

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

[Matthias Gatto]

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 
---
 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
+qemuBuildRAIDFileSourceStr(virConnectPtr conn,
+   virStorageSourcePtr src,
+   virBuffer *opt,
+   const char *toAppend)
+{
+char *source = NULL;
+int actualType = virStorageSourceGetActualType(src);
+
+if (qemuGetDriveSourceString(src, conn, ) < 0)
+return false;
+
+if (source) {
+virBufferStrcat(opt, ",", toAppend, "filename=", NULL);
+
+if (actualType == VIR_STORAGE_TYPE_DIR) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unsupported disk driver type for '%s'"),
+   virStorageFileFormatTypeToString(src->format));
+return false;
+}
+virBufferAdd(opt, source, -1);
+}
+
+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(, "%schildren.%lu.file.", toAppend, i);
+if (ret < 0)
+return false;
+
+virBufferAsprintf(opt, ",%schildren.%lu.driver=%s",
+  toAppend, i,
+  
virStorageFileFormatTypeToString(backingStore->format));
+
+if (qemuBuildRAIDFileSourceStr(conn, backingStore, opt, tmp) == false)
+goto error;
+
+/* This operation avoid us to made another copy */
+tmp[ret - sizeof("file")] = '\0';
+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
@@ -3781,6 +3868,7 @@ qemuBuildDriveStr(virConnectPtr conn,
 goto error;
 
 if (source &&
+!virStorageSourceIsRAID(disk->src) &&
 !((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
   disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
@@ -4132,6 +4220,11 @@ qemuBuildDriveStr(virConnectPtr conn,
   disk->blkdeviotune.size_iops_sec);
 }
 
+if (virStorageSourceIsRAID(disk->src)) {
+if (!qemuBuildRAIDStr(conn, disk, disk->src, , ""))
+goto error;
+}
+
 if (virBufferCheckError() < 0)
 goto error;
 
-- 
2.6.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list