On Wed, Jun 12, 2024 at 03:02:20 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> * Add qemuBuildDiskThrottleFiltersCommandLine in qemuBuildDiskCommandLine to add "blockdev"
> * Make sure referenced throttle group exists
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
> src/conf/domain_validate.c | 12 ++++++++++++
> src/qemu/qemu_command.c | 40 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 27d7a9968b..bedc28d481 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -706,6 +706,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) {
> @@ -952,6 +953,17 @@ virDomainDiskDefValidate(const virDomainDef *def,
> return -1;
> }
>
> + /* referenced group should be defined */
> + for (i = 0; i < disk->nthrottlefilters; i++) {
> + virDomainThrottleFilterDef *filter = disk->throttlefilters[i];
> + if (!virDomainThrottleGroupFind(def, filter->group_name)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("throttle group '%1$s' not found"),
> + filter->group_name);
> + return -1;
> + }
> + }
As noted, please unify all the random bits of validation into single
patch.
> +
> if (disk->throttlefilters && (disk->blkdeviotune.group_name ||
> virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))) {
> virReportError(VIR_ERR_XML_ERROR,
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 863544938f..2194b15fd3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2229,6 +2229,43 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd,
> }
>
>
> +static int
> +qemuBuildBlockThrottleFilterCommandline(virCommand *cmd,
> + qemuBlockThrottleFilterAttachData *data)
> +{
> + if (data->filterProps) {
> + g_autofree char *tmp = NULL;
> + if (!(tmp = virJSONValueToString(data->filterProps, false)))
> + return -1;
> +
> + virCommandAddArgList(cmd, "-blockdev", tmp, NULL);
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> +qemuBuildDiskThrottleFiltersCommandLine(virCommand *cmd,
> + virDomainDiskDef *disk)
> +{
> + g_autoptr(qemuBlockThrottleFiltersData) data = NULL;
> + size_t i;
> +
> + data = qemuBuildThrottleFiltersAttachPrepareBlockdev(disk);
> + if (!data)
> + return -1;
> +
> + 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,
> @@ -2286,6 +2323,9 @@ qemuBuildDiskCommandLine(virCommand *cmd,
> if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
> return -1;
>
> + if (qemuBuildDiskThrottleFiltersCommandLine(cmd, disk) < 0)
> + return -1;
As noted this should be done all at once in one patch. Also as noted I
strongly prefer if this is done along with the images and not here.
Also as noted before this is ordered wrong for how this is supposed to
work as you propose as here the 'copyOnRead' layer is already built.
> +
> /* SD cards are currently instantiated via -drive if=sd, so the -device
> * part must be skipped */
> if (qemuDiskBusIsSD(disk->bus))
> --
> 2.34.1
>
