On Thu, Apr 11, 2024 at 19:01:51 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> Support throttlegroup lifecycle management by the following implementation:
> * New methods defined in "include/libvirt/libvirt-domain.h"
> * And they're exported in "src/libvirt_public.syms"
> * Corresponding internal API is defined in "src/driver-hypervisor.h"
> * Public API calls are implemented in "src/libvirt-domain.c"
> * Wire protocol is defined in "src/remote/remote_protocol.x"
> * RPC client implementation is in "src/remote/remote_driver.c"
> * Server side dispatch is implemented in "src/remote/remote_daemon_dispatch.c"
> * Also "src/remote_protocol-structs" is updated
> * Yu Jie Gu <guyujie@xxxxxxxxxxxxx> helped add
> remote_domain_set_throttle_group_args in src/remote_protocol-structs
> to fix issue reported by check-remote_protocol
Please, rather than describing where you've put the code, use this space
to describe the API itself. Which parameters it takes how it's supposed
to be used.
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
This patch with new APIs really needs to go in after all of the other
code which allows to configure it via XML. I'll for now add some top
level comments and then get back once I see how the XML is designed
first.
> include/libvirt/libvirt-domain.h | 29 +++++
> src/driver-hypervisor.h | 22 ++++
> src/libvirt-domain.c | 190 ++++++++++++++++++++++++++++
> src/libvirt_private.syms | 9 ++
> src/libvirt_public.syms | 7 +
> src/remote/remote_daemon_dispatch.c | 61 +++++++++
> src/remote/remote_driver.c | 49 +++++++
> src/remote/remote_protocol.x | 50 +++++++-
> src/remote_protocol-structs | 30 +++++
> 9 files changed, 446 insertions(+), 1 deletion(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 2f5b01bbfe..5435ab7fcb 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -5593,6 +5593,16 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn,
> */
> # define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH "blkdeviotune.write_iops_sec_max_length"
>
> +/**
> + * VIR_DOMAIN_THROTTLE_GROUP:
> + *
> + * Macro represents the name of throttle group for which the values are updated,
> + * as VIR_TYPED_PARAM_STRING.
> + *
> + * Since: 10.3.0
> + */
> +# define VIR_DOMAIN_THROTTLE_GROUP "throttlegroup.name"
So all of the APIs below take the group name as argument, why do we need
it a s typed parameter?
> +
[...]
> @@ -1726,4 +1745,7 @@ struct _virHypervisorDriver {
> virDrvDomainStartDirtyRateCalc domainStartDirtyRateCalc;
> virDrvDomainFDAssociate domainFDAssociate;
> virDrvDomainGraphicsReload domainGraphicsReload;
> + virDrvDomainSetThrottleGroup domainSetThrottleGroup;
> + virDrvDomainGetThrottleGroup domainGetThrottleGroup;
> + virDrvDomainDelThrottleGroup domainDelThrottleGroup;
> };
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 7c6b93963c..da88457601 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -14162,3 +14162,193 @@ virDomainGraphicsReload(virDomainPtr domain,
> virDispatchError(domain->conn);
> return -1;
> }
> +
> +
> +/**
> + * virDomainSetThrottleGroup:
> + * @dom: pointer to domain object
> + * @group: throttle group name
> + * @params: Pointer to blkio parameter objects
> + * @nparams: Number of blkio parameters (this value can be the same or
> + * less than the number of parameters supported)
> + * @flags: bitwise-OR of virDomainModificationImpact
> + *
> + * add or change throttle group.
Please bee more specific by includingwhich names of parameters are
accepted and/or reference other APIs which do this.
> + *
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + *
> + * Since: 10.3.0
Don't forget to update all of these.
> + */
> +int
> +virDomainSetThrottleGroup(virDomainPtr dom,
> + const char *group,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> + virConnectPtr conn;
> +
> + VIR_DOMAIN_DEBUG(dom, "params=%p, nparams=%d, flags=0x%x",
> + params, nparams, flags);
> + VIR_TYPED_PARAMS_DEBUG(params, nparams);
> +
> + virResetLastError();
> +
> + virCheckDomainReturn(dom, -1);
> + conn = dom->conn;
> +
> + virCheckReadOnlyGoto(conn->flags, error);
> + virCheckPositiveArgGoto(nparams, error);
> + virCheckNonNullArgGoto(params, error);
> +
> + if (virTypedParameterValidateSet(dom->conn, params, nparams) < 0)
> + goto error;
> +
> + if (conn->driver->domainSetThrottleGroup) {
> + int ret;
> + ret = conn->driver->domainSetThrottleGroup(dom, group, params, nparams, flags);
> + if (ret < 0)
> + goto error;
> + return ret;
> + }
> +
> + virReportUnsupportedError();
> +
> + error:
> + virDispatchError(dom->conn);
> + return -1;
> +}
> +
> +
> +/**
> + * virDomainGetThrottleGroup:
> + * @dom: pointer to domain object
> + * @group: throttle group name
> + * @params: Pointer to blkio parameter object
> + * (return value, allocated by the caller)
> + * @nparams: Pointer to number of blkio parameters
> + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
> + *
> + * Get all parameters for specific throttle group. On input,
> + * @nparams gives the size of the @params array; on output, @nparams
> + * gives how many slots were filled with parameter information, which
> + * might be less but will not exceed the input value.
> + *
> + * As a special case, calling with @params as NULL and @nparams as 0
> + * on input will cause @nparams on output to contain the number of
> + * parameters supported by the hypervisor, either for the given @group
> + * or if @group is NULL, for all possible groups. The
> + * caller should then allocate @params array,
> + * i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API
> + * again. See virDomainGetMemoryParameters() for more details.
We no longer do this old-style of APIs which you need to query first for
the number of parameters and then give it a reasonably-sized pointer.
Using it is really hard as it requires multiple calls and can lead to
TOCTOU races.
The new approach is that the API returns allocated array of fields and
number of fileds that are in it.
Historically the idea of handing in a pointer to be filled was that the
hypervisor code was directly running inside the library. This is no
longer the case in most cases as the library just communicates with the
daemon so there will be memory allocations and copies of the data.
> + *
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + *
> + * Since: 10.3.0
> + */
> +int
> +virDomainGetThrottleGroup(virDomainPtr dom,
> + const char *group,
> + virTypedParameterPtr params,
> + int *nparams,
> + unsigned int flags)
> +{
> + virConnectPtr conn;
> + int rc;
> +
> + VIR_DOMAIN_DEBUG(dom, "params=%p, nparams=%d, flags=0x%x",
> + params, (nparams) ? *nparams : -1, flags);
[...]
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
