On Wed, Jun 12, 2024 at 03:02:15 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> 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
Please use shorter lines in commit message.
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
> include/libvirt/libvirt-domain.h | 21 +++
> src/driver-hypervisor.h | 22 ++++
> src/libvirt-domain.c | 196 ++++++++++++++++++++++++++++
> src/libvirt_private.syms | 9 ++
> 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, 414 insertions(+), 1 deletion(-)
[...]
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 7c6b93963c..37ba587b57 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -14162,3 +14162,199 @@ 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
> + *
> + * 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.5.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 and virTypedParameterFlags
Typed parameter flags are not to be passed by user.
> + *
> + * Get all block IO tunable parameters for specific throttle group. @group cannot be NULL.
> + * @nparams gives how many slots were filled with parameter information
> + *
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + *
> + * Since: 10.5.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);
> +
> + virResetLastError();
> +
> + virCheckDomainReturn(dom, -1);
> + virCheckNonNullArgGoto(group, error);
> + virCheckNonNullArgGoto(nparams, error);
> + virCheckNonNullArgGoto(params, error);
> +
> + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn,
> + VIR_DRV_FEATURE_TYPED_PARAM_STRING);
All libvirt versions supporting this API support also string typed
params, so this code is not needed and you can assume support.
> + if (rc < 0)
> + goto error;
> + if (rc)
> + flags |= VIR_TYPED_PARAM_STRING_OKAY;
> +
> + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE,
> + VIR_DOMAIN_AFFECT_CONFIG,
> + error);
You explicitly document that both can be used.
> +
> + 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 and virTypedParameterFlags
This takes not typed params.
> + *
> + * 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.
[1].
> + * 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.5.0
> + */
> +int
> +virDomainDelThrottleGroup(virDomainPtr dom,
> + const char *group,
> + unsigned int flags)
> +{
> + virConnectPtr conn;
> + int rc;
> +
> + virResetLastError();
> +
> + virCheckDomainReturn(dom, -1);
> + virCheckNonNullArgGoto(group, error);
> +
> + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn,
> + VIR_DRV_FEATURE_TYPED_PARAM_STRING);
This API is not passing any typed parameters so all of this code is not
actually used.
> + if (rc < 0)
> + goto error;
> + if (rc)
> + flags |= VIR_TYPED_PARAM_STRING_OKAY;
> +
> + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE,
> + VIR_DOMAIN_AFFECT_CONFIG,
> + error);
This API is updating state of a VM thus must not be allowed on read-only
connections. You'll need to also use proper flags in the .x file (which
I've deleted while replying).
Also note that many of existing APIs allow dual use of _LIVE and _CONFIG
flags. You even document it [1].
> +
> + conn = dom->conn;
> +
> + if (conn->driver->domainDelThrottleGroup) {
> + int ret;
> + ret = conn->driver->domainDelThrottleGroup(dom, group, flags);
> + if (ret < 0)
> + goto error;
> + return ret;
> + }
> +
> + virReportUnsupportedError();
> +
> + error:
> + virDispatchError(dom->conn);
> + return -1;
> +}
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 7a3492d9d7..2b37f29a28 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.5.0 {
10.7.0
> + 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 cfc1067e40..f5a9e45303 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -3614,6 +3614,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,
> + ¶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..4f2b28e662 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;
> + 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) {
> + 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.5.0 */
> + .domainGetThrottleGroup = remoteDomainGetThrottleGroup, /* 10.5.0 */
> + .domainDelThrottleGroup = remoteDomainDelThrottleGroup, /* 10.5.0 */
Don't forget to update these to 10.7.0
> };
>
