On Wed, Jun 12, 2024 at 03:02:19 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> * Add qemuBuildThrottleGroupCommandLine in qemuBuildCommandLine to add "object" of throttle-group
> * Verify throttle group definition when lauching vm
> * Check QEMU_CAPS_OBJECT_JSON before "qemuBuildObjectCommandlineFromJSON", which is to build "-object" option
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
> src/conf/domain_validate.c | 20 +++++++++++++
> src/qemu/qemu_command.c | 58 ++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_command.h | 3 ++
> 3 files changed, 81 insertions(+)
>
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index d724046004..27d7a9968b 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -1818,6 +1818,23 @@ virDomainDefValidateIOThreads(const virDomainDef *def)
> }
>
>
> +static int
> +virDomainDefValidateThrottleGroups(const virDomainDef *def)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + virDomainThrottleGroupDef *throttleGroup = def->throttlegroups[i];
> +
> + /* Validate Throttle Group */
> + if (virDomainDiskIoTuneValidate(*throttleGroup) < 0)
> + return -1;
... this is the validation step ... (see below). This valdiation should
be also added all together with all the other validation bits that I've
seen in other patches and not split randomly.
> + }
> +
> + return 0;
> +}
> +
> +
> static int
> virDomainDefValidateInternal(const virDomainDef *def,
> virDomainXMLOption *xmlopt)
> @@ -1873,6 +1890,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
> if (virDomainDefValidateIOThreads(def) < 0)
> return -1;
>
> + if (virDomainDefValidateThrottleGroups(def) < 0)
> + return -1;
> +
> return 0;
> }
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 5ccae956d3..863544938f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1589,6 +1589,14 @@ qemuDiskConfigThrottleFiltersEnabled(const virDomainDiskDef *disk)
> }
>
>
> +bool
> +qemuDiskConfigThrottleFilterEnabled(const virDomainThrottleGroupDef *group)
Not used anywhere else, avoid export.
> +{
> + return !!group->group_name &&
> + virDomainBlockIoTuneInfoHasAny(group);
> +}
> +
> +
> /**
> * qemuDiskBusIsSD:
> * @bus: disk bus
> @@ -7475,6 +7483,53 @@ 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)
coding style.
> + return -1;
> +
> + if (qemuMonitorCreateObjectProps(&props, "throttle-group", group->group_name,
> + "a:limits", &limits,
> + NULL) < 0)
> + return -1;
> +
> + /* this throttle group feature requires "QEMU_CAPS_OBJECT_JSON" */
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_JSON)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("QEMU_CAPS_OBJECT_JSON support is required for throttle group creation"));
> + return -1;
This belongs to the validation step and not to the command line building
step. Also same comment as before about not using internal error and
internal flag names, which are meaningless to the users.
> + }
> + if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> static int
> qemuBuildNumaCellCache(virCommand *cmd,
[...]
