Re: [PATCH RFC v2 03/12] remote: New APIs for ThrottleGroup lifecycle management

On Thu, Apr 11, 2024 at 19:01:51 -0700, wucf(a)linux.ibm.com wrote: 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. 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. 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. 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. > + */ > +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); [...]