On Mon, Nov 18, 2024 at 19:24:15 +0530, Harikumar R wrote:
> From: Chun Feng Wu <danielwuwy@xxxxxxx>
>
> Defined new public APIs:
> * virDomainSetThrottleGroup to add or update throttlegroup within specific domain,
> it will be referenced by throttlefilter later in disk to do limits
> * virDomainGetThrottleGroup to get throttlegroup info, old-style is discarded
> (APIs to query first for the number of parameters and then give it a
> reasonably-sized pointer), instead, the new approach is adopted that
> API returns allocated array of fields and number of fileds that are in it.
> * virDomainDelThrottleGroup to delete throttlegroup, it fails if this throttlegroup
> is still referenced by some throttlefilter
>
> Signed-off-by: Chun Feng Wu <danielwuwy@xxxxxxx>
> ---
> include/libvirt/libvirt-domain.h | 21 ++++
> src/driver-hypervisor.h | 22 ++++
> src/libvirt-domain.c | 174 ++++++++++++++++++++++++++++
> src/libvirt_private.syms | 8 ++
> src/libvirt_public.syms | 7 ++
> src/remote/remote_daemon_dispatch.c | 44 +++++++
> src/remote/remote_driver.c | 40 +++++++
> src/remote/remote_protocol.x | 48 +++++++-
> src/remote_protocol-structs | 28 +++++
> 9 files changed, 391 insertions(+), 1 deletion(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 9232ce2e6b..7ff1e50ef5 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -6560,4 +6560,25 @@ virDomainGraphicsReload(virDomainPtr domain,
> unsigned int type,
> unsigned int flags);
>
> +
> +int
> +virDomainSetThrottleGroup(virDomainPtr dom,
> + const char *group,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags);
> +
> +int
> +virDomainGetThrottleGroup(virDomainPtr dom,
> + const char *group,
> + virTypedParameterPtr *params,
> + int *nparams,
> + unsigned int flags);
I'm not exactly a fan of 'getter' functions for configuration which is
also present in the XML, but since we did these historically I'm okay
with this.
[...]
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 7c6b93963c..ce18d18854 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -14162,3 +14162,177 @@ 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 throttlegroup or change all or a subset of the throttlegroup options
> + * within specific domain.
I don't think we should do the 'change a subset of' semantics any more.
It was proven that it doesn't make sense. Specifically it's not possible
to distinguish between "I don't want to change this field" and "I want
to delete/revert to default for this field".
The users must be instructed to always post the full required config.
> + *
> + * The @group parameter is the name for new or existing throttlegroup,
> + * it cannot be NULL, detailed throttlegroup info is included in @params,
> + * it either creates new throttlegroup with @params or updates existing
> + * throttlegroup with @params, throttlegroup can be referenced by throttle
> + * filter in attached disk to do limits, the difference from iotune is that
> + * multiple throttlegroups can be referenced within attached disk
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + *
> + * Since: 10.7.0
11.1.0
> + */
> +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);
> + virCheckNonNullArgGoto(group, 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 that will be filled with an array of typed parameters
> + * @nparams: pointer filled with number of elements in @params
> + * @flags: bitwise-OR of virDomainModificationImpact
> + *
> + * Get all block IO tunable parameters for specific throttle group. @group cannot be NULL.
> + * @nparams gives how many slots were filled with parameter information
> + * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG.
> + * Both flags may be set.
For getter APIs you can't set both because there's no way the API can
return both configs at once.
> + *
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + *
> + * Since: 10.7.0
> + */
> +int
> +virDomainGetThrottleGroup(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) ? *nparams : -1, flags);
The actual value of 'nparams' at call doesn't matter, thus makes no
sense to be logged. At the same time, 'group' which is a much more
useful info is not logged.
> +
> + virResetLastError();
> +
> + virCheckDomainReturn(dom, -1);
> + virCheckNonNullArgGoto(group, error);
> + virCheckNonNullArgGoto(nparams, error);
> + virCheckNonNullArgGoto(params, error);
> +
> + conn = dom->conn;
> +
> + if (conn->driver->domainGetThrottleGroup) {
> + int ret;
> + ret = conn->driver->domainGetThrottleGroup(dom, group, params, nparams, flags);
> + if (ret < 0)
> + goto error;
> + return ret;
> + }
> +
> + virReportUnsupportedError();
> +
> + error:
> + virDispatchError(dom->conn);
> + return -1;
> +}
> +
> +
> +/**
> + * virDomainDelThrottleGroup:
> + * @dom: pointer to domain object
> + * @group: throttle group name
> + * @flags: bitwise-OR of virDomainModificationImpact
> + *
> + * Delete an throttlegroup from the domain. @group cannot be NULL,
> + * and the @group to be deleted must not have a throttlefilter associated with
> + * it and can be any of the current valid group.
> + *
> + * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG.
> + * Both flags may be set.
> + * If VIR_DOMAIN_AFFECT_LIVE is set, the change affects a running domain
> + * and may fail if domain is not alive.
> + * If VIR_DOMAIN_AFFECT_CONFIG is set, the change affects persistent state,
> + * and will fail for transient domains. If neither flag is specified (that is,
> + * @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain modifies
> + * persistent setup, while an active domain is hypervisor-dependent on whether
> + * just live or both live and persistent state is changed.
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + *
> + * Since: 10.7.0
> + */
> +int
> +virDomainDelThrottleGroup(virDomainPtr dom,
> + const char *group,
> + unsigned int flags)
> +{
> + virConnectPtr conn;
> +
> + virResetLastError();
> +
> + virCheckDomainReturn(dom, -1);
> + virCheckNonNullArgGoto(group, error);
> +
[...]
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 7a3492d9d7..f17b6feaef 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -948,4 +948,11 @@ LIBVIRT_10.2.0 {
> virDomainGraphicsReload;
> } LIBVIRT_10.1.0;
>
> +LIBVIRT_10.7.0 {
This also must correspond to the proper version and this one can't be
fixed later.
> + global:
> + virDomainSetThrottleGroup;
> + virDomainGetThrottleGroup;
> + virDomainDelThrottleGroup;
> +} LIBVIRT_10.2.0;
> +
> # .... define new API here using predicted next version number ....
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index e812f5c3e9..5d5f286203 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -3618,6 +3618,50 @@ remoteDispatchDomainGetBlockIoTune(virNetServer *server G_GNUC_UNUSED,
> return rv;
> }
>
> +
> +static int
> +remoteDispatchDomainGetThrottleGroup(virNetServer *server G_GNUC_UNUSED,
> + virNetServerClient *client,
> + virNetMessage *hdr G_GNUC_UNUSED,
> + struct virNetMessageError *rerr,
> + remote_domain_get_throttle_group_args *args,
> + remote_domain_get_throttle_group_ret *ret)
> +{
> + virDomainPtr dom = NULL;
> + int rv = -1;
> + virTypedParameterPtr params = NULL;
> + int nparams = 0;
> + virConnectPtr conn = remoteGetHypervisorConn(client);
> +
> + if (!conn)
> + goto cleanup;
> +
> + if (!(dom = get_nonnull_domain(conn, args->dom)))
> + goto cleanup;
> +
> + if (virDomainGetThrottleGroup(dom, args->group ? *args->group : NULL,
This is a weird construction, 'group' is supposed to be a nonnull
string, so why should it ever be NULL?
> + ¶ms, &nparams, args->flags) < 0)
> + goto cleanup;
> +
> + /* Serialize the ThrottleGroup parameters. */
> + if (virTypedParamsSerialize(params, nparams,
> + REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX,
> + (struct _virTypedParameterRemote **) &ret->params.params_val,
> + &ret->params.params_len,
> + args->flags) < 0)
> + goto cleanup;
> +
> + rv = 0;
> +
> + cleanup:
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> + virTypedParamsFree(params, nparams);
> + virObjectUnref(dom);
> + return rv;
> +}
> +
> +
> /*-------------------------------------------------------------*/
>
> static int
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index e76d9e9ba4..c757156e24 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -2583,6 +2583,43 @@ static int remoteDomainGetBlockIoTune(virDomainPtr domain,
> return 0;
> }
>
> +
> +static int
> +remoteDomainGetThrottleGroup(virDomainPtr domain,
> + const char *group,
> + virTypedParameterPtr *params,
> + int *nparams,
> + unsigned int flags)
> +{
> + remote_domain_get_throttle_group_args args = {0};
> + g_auto(remote_domain_get_throttle_group_ret) ret = {0};
> + struct private_data *priv = domain->conn->privateData;
> + VIR_LOCK_GUARD lock = remoteDriverLock(priv);
> +
> + make_nonnull_domain(&args.dom, domain);
> + args.group = group ? (char **)&group : NULL;
Spacing broken. Also 'group' is a mandatory argument, so why it's being
checked? This line makes no sense to me.
> + args.flags = flags;
> +
> + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_THROTTLE_GROUP,
> + (xdrproc_t) xdr_remote_domain_get_throttle_group_args,
> + (char *) &args,
> + (xdrproc_t) xdr_remote_domain_get_throttle_group_ret,
> + (char *) &ret) == -1) {
Alignment of args seems to be broken.
> + return -1;
> + }
> +
> + if (virTypedParamsDeserialize((struct _virTypedParameterRemote *) ret.params.params_val,
> + ret.params.params_len,
> + REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX,
> + params,
> + nparams) < 0)
> + return -1;
> +
> + return 0;
> +
> +}
> +
> +
> static int remoteDomainGetCPUStats(virDomainPtr domain,
> virTypedParameterPtr params,
> unsigned int nparams,
> @@ -7842,6 +7879,9 @@ static virHypervisorDriver hypervisor_driver = {
> .domainSetLaunchSecurityState = remoteDomainSetLaunchSecurityState, /* 8.0.0 */
> .domainFDAssociate = remoteDomainFDAssociate, /* 9.0.0 */
> .domainGraphicsReload = remoteDomainGraphicsReload, /* 10.2.0 */
> + .domainSetThrottleGroup = remoteDomainSetThrottleGroup, /* 10.7.0 */
> + .domainGetThrottleGroup = remoteDomainGetThrottleGroup, /* 10.7.0 */
> + .domainDelThrottleGroup = remoteDomainDelThrottleGroup, /* 10.7.0 */
11.1.0
> };
>
> static virNetworkDriver network_driver = {
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 41c045ff78..02338b9f02 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -113,6 +113,9 @@ const REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX = 16;
> /* Upper limit on list of blockio tuning parameters. */
> const REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX = 32;
>
> +/* Upper limit on list of throttle group parameters. */
> +const REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX = 32;
> +
> /* Upper limit on list of numa parameters. */
> const REMOTE_DOMAIN_NUMA_PARAMETERS_MAX = 16;
>
> @@ -1475,6 +1478,29 @@ struct remote_domain_get_block_io_tune_ret {
> int nparams;
> };
>
> +struct remote_domain_set_throttle_group_args {
> + remote_nonnull_domain dom;
> + remote_nonnull_string group;
> + remote_typed_param params<REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX>;
> + unsigned int flags;
> +};
> +
> +struct remote_domain_get_throttle_group_args {
> + remote_nonnull_domain dom;
> + remote_string group;
> + unsigned int flags;
> +};
> +
> +struct remote_domain_get_throttle_group_ret {
> + remote_typed_param params<REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX>;
> +};
> +
> +struct remote_domain_del_throttle_group_args {
> + remote_nonnull_domain dom;
> + remote_string group;
> + unsigned int flags;
> +};
> +
> struct remote_domain_get_cpu_stats_args {
> remote_nonnull_domain dom;
> unsigned int nparams;
> @@ -7048,5 +7074,25 @@ enum remote_procedure {
> * @generate: both
> * @acl: domain:write
> */
> - REMOTE_PROC_DOMAIN_GRAPHICS_RELOAD = 448
> + REMOTE_PROC_DOMAIN_GRAPHICS_RELOAD = 448,
> +
> + /**
> + * @generate: both
> + * @acl: domain:write
> + * @acl: domain:save:!VIR_DOMAIN_AFFECT_CONFIG|VIR_DOMAIN_AFFECT_LIVE
> + * @acl: domain:save:VIR_DOMAIN_AFFECT_CONFIG
> + */
> + REMOTE_PROC_DOMAIN_SET_THROTTLE_GROUP = 449,
> +
> + /**
> + * @generate: none
> + * @acl: domain:read
> + */
> + REMOTE_PROC_DOMAIN_GET_THROTTLE_GROUP = 450,
> +
> + /**
> + * @generate: both
> + * @acl: domain:write
Deletion of the throttle group needs to have same ACLs as the SET, thus
also the 'save' perms need to be copied.
> + */
> + REMOTE_PROC_DOMAIN_DEL_THROTTLE_GROUP = 451
> };
