From: Chun Feng Wu <wucf@linux.ibm.com>

* Add qemuBuildThrottleGroupCommandLine in qemuBuildCommandLine
* Add qemuBuildThrottleFiltersCommandLine in qemuBuildDiskCommandLine
* Make sure referenced throttle group exists

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com>
---
 src/conf/domain_validate.c | 14 ++++++
 src/qemu/qemu_command.c    | 94 ++++++++++++++++++++++++++++++++++++++
 src/qemu/qemu_command.h    |  3 ++
 3 files changed, 111 insertions(+)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 395e036e8f..fffe274afc 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -663,6 +663,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
                          const virDomainDiskDef *disk)
 {
     virStorageSource *next;
+    size_t i;
 
     /* disk target is used widely in other code so it must be validated first */
     if (!disk->dst) {
@@ -942,6 +943,19 @@ virDomainDiskDefValidate(const virDomainDef *def,
         return -1;
     }
 
+    /* referenced group should be defined */
+    for (i = 0; i < disk->nthrottlefilters; i++) {
+        virDomainThrottleFilterDef *filter = disk->throttlefilters[i];
+        if (filter) {
+            if (!virDomainThrottleGroupFind(def, filter->group_name)) {
+                virReportError(VIR_ERR_XML_ERROR,
+                            _("throttle group '%1$s' not found"),
+                            filter->group_name);
+                return -1;
+            }
+        }
+    }
+
     return 0;
 }
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2d8036c3ae..34164c098b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1584,6 +1584,14 @@ qemuDiskConfigThrottleFilterChainEnabled(const virDomainDiskDef *disk)
 }
 
 
+bool
+qemuDiskConfigThrottleFilterEnabled(const virDomainThrottleGroupDef *group)
+{
+    return !!group->group_name &&
+           virDomainBlockIoTuneInfoHasAny(group);
+}
+
+
 /**
  * qemuDiskBusIsSD:
  * @bus: disk bus
@@ -2221,6 +2229,45 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd,
 }
 
 
+static int
+qemuBuildBlockThrottleFilterCommandline(virCommand *cmd,
+                                        qemuBlockThrottleFilterAttachData *data)
+{
+    char *tmp;
+
+    if (data->filterProps) {
+        if (!(tmp = virJSONValueToString(data->filterProps, false)))
+            return -1;
+
+        virCommandAddArgList(cmd, "-blockdev", tmp, NULL);
+        VIR_FREE(tmp);
+    }
+
+    return 0;
+}
+
+
+static int
+qemuBuildThrottleFiltersCommandLine(virCommand *cmd,
+                                    virDomainDiskDef *disk)
+{
+    g_autoptr(qemuBlockThrottleFilterChainData) data = NULL;
+    size_t i;
+
+    if (!(data = qemuBuildThrottleFilterChainAttachPrepareBlockdev(disk))) {
+        return -1;
+    } else {
+        for (i = 0; i < data->nfilterdata; i++) {
+            if (qemuBuildBlockThrottleFilterCommandline(cmd,
+                                                        data->filterdata[i]) < 0)
+                return -1;
+        }
+    }
+
+    return 0;
+}
+
+
 static int
 qemuBuildDiskSourceCommandLine(virCommand *cmd,
                                virDomainDiskDef *disk,
@@ -2278,6 +2325,9 @@ qemuBuildDiskCommandLine(virCommand *cmd,
     if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
         return -1;
 
+    if (qemuBuildThrottleFiltersCommandLine(cmd, disk) < 0)
+        return -1;
+
     /* SD cards are currently instantiated via -drive if=sd, so the -device
      * part must be skipped */
     if (qemuDiskBusIsSD(disk->bus))
@@ -7451,6 +7501,47 @@ qemuBuildIOThreadCommandLine(virCommand *cmd,
 }
 
 
+/**
+ * qemuBuildThrottleGroupCommandLine:
+ * @cmd: the command to modify
+ * @def: domain definition
+ * @qemuCaps: qemu capabilities object
+ *
+ * build throttle group object in json format
+ * e.g. -object '{"qom-type":"throttle-group","id":"limit0","limits":{"iops-total":200}}'
+ */
+static int
+qemuBuildThrottleGroupCommandLine(virCommand *cmd,
+                                  const virDomainDef *def,
+                                  virQEMUCaps *qemuCaps)
+{
+    size_t i;
+
+    for (i = 0; i < def->nthrottlegroups; i++) {
+        g_autoptr(virJSONValue) props = NULL;
+        g_autoptr(virJSONValue) limits = virJSONValueNewObject();
+        virDomainThrottleGroupDef *group = def->throttlegroups[i];
+
+        if (!qemuDiskConfigThrottleFilterEnabled(group)) {
+            continue;
+        }
+
+        if (qemuMonitorThrottleGroupLimits(limits, group)<0)
+            return -1;
+
+        if (qemuMonitorCreateObjectProps(&props, "throttle-group", group->group_name,
+                                         "a:limits", &limits,
+                                         NULL) < 0)
+            return -1;
+
+        if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuBuildNumaCellCache(virCommand *cmd,
                        const virDomainDef *def,
@@ -10485,6 +10576,9 @@ qemuBuildCommandLine(virDomainObj *vm,
     if (qemuBuildIOThreadCommandLine(cmd, def, qemuCaps) < 0)
         return NULL;
 
+    if (qemuBuildThrottleGroupCommandLine(cmd, def, qemuCaps) < 0)
+        return NULL;
+
     if (virDomainNumaGetNodeCount(def->numa) &&
         qemuBuildNumaCommandLine(cfg, def, cmd, priv) < 0)
         return NULL;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index e2dee47906..9b8951d95f 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -122,6 +122,9 @@ qemuBuildThrottleFilterChainAttachPrepareBlockdev(virDomainDiskDef *disk);
 bool
 qemuDiskConfigThrottleFilterChainEnabled(const virDomainDiskDef *disk);
 
+bool
+qemuDiskConfigThrottleFilterEnabled(const virDomainThrottleGroupDef *group);
+
 virJSONValue *
 qemuBuildDiskDeviceProps(const virDomainDef *def,
                          virDomainDiskDef *disk,
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Apr 11, 2024 at 19:01:54 -0700, wucf@linux.ibm.com wrote:
> From: Chun Feng Wu <wucf@linux.ibm.com>
> 
> * Add qemuBuildThrottleGroupCommandLine in qemuBuildCommandLine
> * Add qemuBuildThrottleFiltersCommandLine in qemuBuildDiskCommandLine
> * Make sure referenced throttle group exists
> 
> Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com>
> ---

Similarly to previous patches this should be after the patch adding XML
schema so that it's obvious what the design is first.

Additionally you mix the thottle group definition code with the disk
filter code again. It makes this series very unpleasant to review as
it's mixing contexts.

> 
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 395e036e8f..fffe274afc 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -663,6 +663,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
>                           const virDomainDiskDef *disk)
>  {
>      virStorageSource *next;
> +    size_t i;
>  
>      /* disk target is used widely in other code so it must be validated first */
>      if (!disk->dst) {
> @@ -942,6 +943,19 @@ virDomainDiskDefValidate(const virDomainDef *def,
>          return -1;
>      }
>  
> +    /* referenced group should be defined */
> +    for (i = 0; i < disk->nthrottlefilters; i++) {
> +        virDomainThrottleFilterDef *filter = disk->throttlefilters[i];
> +        if (filter) {

Can this even be NULL?

> +            if (!virDomainThrottleGroupFind(def, filter->group_name)) {
> +                virReportError(VIR_ERR_XML_ERROR,
> +                            _("throttle group '%1$s' not found"),
> +                            filter->group_name);
> +                return -1;
> +            }
> +        }
> +    }

Additionally this validation is insufficient.
'qemuDiskConfigThrottleFilterEnabled' below skips the formatting of the
throttle group object if it has no config, but present name.

This code will accept it but qemu will most likely reject it as the
group was not defined.

I've also seen the statement that old-style throttling should not be
combined with this approach. I'm missing a check that forbids such
configs.

> +
>      return 0;
>  }
>  
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2d8036c3ae..34164c098b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1584,6 +1584,14 @@ qemuDiskConfigThrottleFilterChainEnabled(const virDomainDiskDef *disk)
>  }
>  
>  
> +bool
> +qemuDiskConfigThrottleFilterEnabled(const virDomainThrottleGroupDef *group)
> +{
> +    return !!group->group_name &&
> +           virDomainBlockIoTuneInfoHasAny(group);
> +}
> +
> +
>  /**
>   * qemuDiskBusIsSD:
>   * @bus: disk bus
> @@ -2221,6 +2229,45 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd,
>  }
>  
>  
> +static int
> +qemuBuildBlockThrottleFilterCommandline(virCommand *cmd,
> +                                        qemuBlockThrottleFilterAttachData *data)
> +{
> +    char *tmp;
> +
> +    if (data->filterProps) {
> +        if (!(tmp = virJSONValueToString(data->filterProps, false)))
> +            return -1;
> +
> +        virCommandAddArgList(cmd, "-blockdev", tmp, NULL);
> +        VIR_FREE(tmp);

Declare 'tmp' inside the loop with g_autofree instead of this manual
thing.

> +    }
> +
> +    return 0;
> +}
> +
> +
> +static int
> +qemuBuildThrottleFiltersCommandLine(virCommand *cmd,
> +                                    virDomainDiskDef *disk)
> +{
> +    g_autoptr(qemuBlockThrottleFilterChainData) data = NULL;
> +    size_t i;
> +
> +    if (!(data = qemuBuildThrottleFilterChainAttachPrepareBlockdev(disk))) {
> +        return -1;
> +    } else {

Don't do these compound conditions. Always directly return on error and
leave the success code paths in the main control flow. It's interrupting
the understanding of the code.

> +        for (i = 0; i < data->nfilterdata; i++) {
> +            if (qemuBuildBlockThrottleFilterCommandline(cmd,
> +                                                        data->filterdata[i]) < 0)
> +                return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuBuildDiskSourceCommandLine(virCommand *cmd,
>                                 virDomainDiskDef *disk,
> @@ -2278,6 +2325,9 @@ qemuBuildDiskCommandLine(virCommand *cmd,
>      if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
>          return -1;
>  
> +    if (qemuBuildThrottleFiltersCommandLine(cmd, disk) < 0)
> +        return -1;

The name should include 'Disk'.

> +
>      /* SD cards are currently instantiated via -drive if=sd, so the -device
>       * part must be skipped */
>      if (qemuDiskBusIsSD(disk->bus))
> @@ -7451,6 +7501,47 @@ qemuBuildIOThreadCommandLine(virCommand *cmd,
>  }
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org