On Thu, Apr 11, 2024 at 19:01:54 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> * Add qemuBuildThrottleGroupCommandLine in qemuBuildCommandLine
> * Add qemuBuildThrottleFiltersCommandLine in qemuBuildDiskCommandLine
> * Make sure referenced throttle group exists
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
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@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
