Re: [PATCH RFC v3 08/16] qemu: Implement qemu driver for throttle API

On Wed, Jun 12, 2024 at 03:02:16 -0700, wucf(a)linux.ibm.com wrote: > From: Chun Feng Wu <wucf(a)linux.ibm.com&gt; > > Implement the following methods in qemu driver: > * Extract common methods for "qemuDomainSetBlockIoTune" and "qemuDomainSetThrottleGroup": qemuDomainValidateBlockIoTune, qemuDomainSetBlockIoTuneFields, qemuDomainCheckBlockIoTuneMutualExclusion, qemuDomainCheckBlockIoTuneMax. > * "qemuDomainSetThrottleGroup", this method is to add("object-add") or update("qom-set") throttlegroup in QOM and update corresponding objects in DOM > * "qemuDomainGetThrottleGroup", this method queries throttlegroup info by groupname > * "qemuDomainDelThrottleGroup", this method checks if group is referenced by any throttle in disks and delete it if it's not used anymore > * Check flag "QEMU_CAPS_OBJECT_JSON" during qemuDomainSetThrottleGroup, throttle group feature requries such flag > * "objectAddNoWrap"("props") check is done by reusing qemuMonitorAddObject in qemuDomainSetThrottleGroup Same comment as before. Try to keep the lines shorter and preferrably do a high level description rather than repeating what the commit does. > Signed-off-by: Chun Feng Wu <wucf(a)linux.ibm.com&gt; > --- > src/qemu/qemu_driver.c | 611 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 528 insertions(+), 83 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index e2698c7924..f7e435d6d5 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -14955,35 +14955,8 @@ qemuDomainCheckBlockIoTuneReset(virDomainDiskDef *disk, > > > static int > -qemuDomainSetBlockIoTune(virDomainPtr dom, > - const char *path, > - virTypedParameterPtr params, > - int nparams, > - unsigned int flags) > +qemuDomainValidateBlockIoTune(virTypedParameterPtr params, int nparams) This refactor should be separated to a separate commit as this commit is getting a bit too complex. > { > - virQEMUDriver *driver = dom->conn->privateData; > - virDomainObj *vm = NULL; > - qemuDomainObjPrivate *priv; > - virDomainDef *def = NULL; > - virDomainDef *persistentDef = NULL; > - virDomainBlockIoTuneInfo info = { 0 }; > - virDomainBlockIoTuneInfo conf_info = { 0 }; > - int ret = -1; > - size_t i; > - virDomainDiskDef *conf_disk = NULL; > - virDomainDiskDef *disk; > - qemuBlockIoTuneSetFlags set_fields = 0; > - g_autoptr(virQEMUDriverConfig) cfg = NULL; > - virObjectEvent *event = NULL; > - virTypedParameterPtr eventParams = NULL; > - int eventNparams = 0; > - int eventMaxparams = 0; > - virDomainBlockIoTuneInfo *cur_info; > - virDomainBlockIoTuneInfo *conf_cur_info; > - > - > - virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > - VIR_DOMAIN_AFFECT_CONFIG, -1); > if (virTypedParamsValidate(params, nparams, > VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, > VIR_TYPED_PARAM_ULLONG, > @@ -15028,32 +15001,28 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > NULL) < 0) > return -1; > > - if (!(vm = qemuDomainObjFromDomain(dom))) > - return -1; > - > - if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0) > - goto cleanup; > - > - cfg = virQEMUDriverGetConfig(driver); > - > - if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) > - goto cleanup; > - > - priv = vm->privateData; > + return 0; > +} > > - if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) > - goto endjob; > > - if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, > - VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) > - goto endjob; > +static int > +qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info, > + virTypedParameterPtr params, > + int nparams, > + qemuBlockIoTuneSetFlags *set_fields, > + virTypedParameterPtr *eventParams, > + int *eventNparams, > + int *eventMaxparams) > +{ > + size_t i; > + int ret = -1; > > #define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ > if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ > - info.FIELD = param->value.ul; \ > - set_fields |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \ > - if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ > - &eventMaxparams, \ > + info->FIELD = param->value.ul; \ > + *set_fields |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \ > + if (virTypedParamsAddULLong(eventParams, eventNparams, \ > + eventMaxparams, \ > VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ > param->value.ul) < 0) \ > goto endjob; \ > @@ -15093,10 +15062,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > > /* NB: Cannot use macro since this is a value.s not a value.ul */ > if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { > - info.group_name = g_strdup(param->value.s); > - set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; > - if (virTypedParamsAddString(&eventParams, &eventNparams, > - &eventMaxparams, > + info->group_name = g_strdup(param->value.s); > + *set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; > + if (virTypedParamsAddString(eventParams, eventNparams, > + eventMaxparams, > VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, > param->value.s) < 0) > goto endjob; > @@ -15119,56 +15088,56 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > > #undef SET_IOTUNE_FIELD > > - if ((info.total_bytes_sec && info.read_bytes_sec) || > - (info.total_bytes_sec && info.write_bytes_sec)) { > + ret = 0; > + endjob: > + return ret; > +} > + > + > +static int > +qemuDomainCheckBlockIoTuneMutualExclusion(virDomainBlockIoTuneInfo *info) > +{ > + if ((info->total_bytes_sec && info->read_bytes_sec) || > + (info->total_bytes_sec && info->write_bytes_sec)) { > virReportError(VIR_ERR_INVALID_ARG, "%s", > _("total and read/write of bytes_sec cannot be set at the same time")); > - goto endjob; > + return -1; > } > > - if ((info.total_iops_sec && info.read_iops_sec) || > - (info.total_iops_sec && info.write_iops_sec)) { > + if ((info->total_iops_sec && info->read_iops_sec) || > + (info->total_iops_sec && info->write_iops_sec)) { > virReportError(VIR_ERR_INVALID_ARG, "%s", > _("total and read/write of iops_sec cannot be set at the same time")); > - goto endjob; > + return -1; > } > > - if ((info.total_bytes_sec_max && info.read_bytes_sec_max) || > - (info.total_bytes_sec_max && info.write_bytes_sec_max)) { > + if ((info->total_bytes_sec_max && info->read_bytes_sec_max) || > + (info->total_bytes_sec_max && info->write_bytes_sec_max)) { > virReportError(VIR_ERR_INVALID_ARG, "%s", > _("total and read/write of bytes_sec_max cannot be set at the same time")); > - goto endjob; > + return -1; > } > > - if ((info.total_iops_sec_max && info.read_iops_sec_max) || > - (info.total_iops_sec_max && info.write_iops_sec_max)) { > + if ((info->total_iops_sec_max && info->read_iops_sec_max) || > + (info->total_iops_sec_max && info->write_iops_sec_max)) { > virReportError(VIR_ERR_INVALID_ARG, "%s", > _("total and read/write of iops_sec_max cannot be set at the same time")); > - goto endjob; > + return -1; > } > > - virDomainBlockIoTuneInfoCopy(&info, &conf_info); > - > - if (def) { > - if (!(disk = qemuDomainDiskByName(def, path))) > - goto endjob; > - > - if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) > - goto endjob; > + return 0; > +} > > - cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info); > > - if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, > - set_fields) < 0) > - goto endjob; > - > - if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0) > - goto endjob; > +static int > +qemuDomainCheckBlockIoTuneMax(virDomainBlockIoTuneInfo *info) > +{ > + int ret = -1; > > #define CHECK_MAX(val, _bool) \ > do { \ > - if (info.val##_max) { \ > - if (!info.val) { \ > + if (info->val##_max) { \ > + if (!info->val) { \ > if (QEMU_BLOCK_IOTUNE_SET_##_bool) { \ > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ > _("cannot reset '%1$s' when '%2$s' is set"), \ > @@ -15180,7 +15149,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > } \ > goto endjob; \ > } \ > - if (info.val##_max < info.val) { \ > + if (info->val##_max < info->val) { \ > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ > _("value '%1$s' cannot be smaller than '%2$s'"), \ > #val "_max", #val); \ > @@ -15198,6 +15167,96 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > > #undef CHECK_MAX > > + ret = 0; > + endjob: > + return ret; > +} > + > + > +static int > +qemuDomainSetBlockIoTune(virDomainPtr dom, > + const char *path, > + virTypedParameterPtr params, > + int nparams, > + unsigned int flags) > +{ > + virQEMUDriver *driver = dom->conn->privateData; > + virDomainObj *vm = NULL; > + qemuDomainObjPrivate *priv; > + virDomainDef *def = NULL; > + virDomainDef *persistentDef = NULL; > + virDomainBlockIoTuneInfo info = { 0 }; > + virDomainBlockIoTuneInfo conf_info = { 0 }; > + int ret = -1; > + virDomainDiskDef *conf_disk = NULL; > + virDomainDiskDef *disk; > + qemuBlockIoTuneSetFlags set_fields = 0; > + g_autoptr(virQEMUDriverConfig) cfg = NULL; > + virObjectEvent *event = NULL; > + virTypedParameterPtr eventParams = NULL; > + int eventNparams = 0; > + int eventMaxparams = 0; > + virDomainBlockIoTuneInfo *cur_info; > + virDomainBlockIoTuneInfo *conf_cur_info; > + > + > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > + VIR_DOMAIN_AFFECT_CONFIG, -1); > + if (qemuDomainValidateBlockIoTune(params, nparams) < 0) > + return -1; > + > + if (!(vm = qemuDomainObjFromDomain(dom))) > + return -1; > + > + if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0) > + goto cleanup; > + > + cfg = virQEMUDriverGetConfig(driver); > + > + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) > + goto cleanup; > + > + priv = vm->privateData; > + > + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) > + goto endjob; > + > + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, > + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) > + goto endjob; > + > + if (qemuDomainSetBlockIoTuneFields(&info, > + params, > + nparams, > + &set_fields, > + &eventParams, > + &eventNparams, > + &eventMaxparams) < 0) > + goto endjob; > + > + if (qemuDomainCheckBlockIoTuneMutualExclusion(&info) < 0) > + goto endjob; > + > + virDomainBlockIoTuneInfoCopy(&info, &conf_info); > + > + if (def) { > + if (!(disk = qemuDomainDiskByName(def, path))) > + goto endjob; > + > + if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) > + goto endjob; > + > + cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info); > + > + if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, set_fields) < 0) > + goto endjob; > + > + if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0) > + goto endjob; > + > + if (qemuDomainCheckBlockIoTuneMax(&info) < 0) > + goto endjob; > + > /* blockdev-based qemu doesn't want to set the throttling when a cdrom > * is empty. Skip the monitor call here since we will set the throttling > * once new media is inserted */ All of the above belongs to a separate or two separate commits. > @@ -19995,6 +20054,389 @@ qemuDomainGraphicsReload(virDomainPtr domain, > return ret; > } > > + > +/** > + * qemuDomainCheckThrottleGroupReset: > + * @groupname: group to create or update > + * @newiotune: Pointer to iotune, which contains detailed items > + * > + * Check if params within @newiotune contain all zero values, if yes > + * and @newiotune specifies the same group name as @groupname, return > + * failure since it's meaningless to set all zero values in @newiotune > + * > + * Returns -1 on failure, or 0 on success. > + */ > +static int > +qemuDomainCheckThrottleGroupReset(const char *groupname, > + virDomainBlockIoTuneInfo *newiotune) > +{ > + if (virDomainBlockIoTuneInfoHasAny(newiotune)) > + return 0; > + > + if (newiotune->group_name && > + STRNEQ_NULLABLE(newiotune->group_name, groupname)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("creating a new group/updating existing with all parameters zero is not supported")); > + return -1; > + } > + > + /* all zero means remove any throttling and remove from group for qemu */ > + VIR_FREE(newiotune->group_name); > + > + return 0; > +} > + > + > +static int > +qemuDomainSetThrottleGroup(virDomainPtr dom, > + const char *groupname, > + virTypedParameterPtr params, > + int nparams, > + unsigned int flags) > +{ > + virQEMUDriver *driver = dom->conn->privateData; > + virDomainObj *vm = NULL; > + virDomainDef *def = NULL; > + virDomainDef *persistentDef = NULL; > + virDomainThrottleGroupDef info = { 0 }; > + virDomainThrottleGroupDef conf_info = { 0 }; > + int ret = -1; > + qemuBlockIoTuneSetFlags set_fields = 0; > + g_autoptr(virQEMUDriverConfig) cfg = NULL; > + virObjectEvent *event = NULL; > + virTypedParameterPtr eventParams = NULL; > + int eventNparams = 0; > + int eventMaxparams = 0; > + virDomainThrottleGroupDef *cur_info; > + virDomainThrottleGroupDef *conf_cur_info; > + int rc = 0; > + g_autoptr(virJSONValue) props = NULL; > + g_autoptr(virJSONValue) limits = virJSONValueNewObject(); > + qemuDomainObjPrivate *priv = NULL; > + virQEMUCaps *qemuCaps = NULL; > + > + > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > + VIR_DOMAIN_AFFECT_CONFIG, -1); > + if (qemuDomainValidateBlockIoTune(params, nparams) < 0) > + return -1; Spacing. > + > + if (!(vm = qemuDomainObjFromDomain(dom))) > + return -1; > + > + if (virDomainSetThrottleGroupEnsureACL(dom->conn, vm->def, flags) < 0) > + goto cleanup; > + > + cfg = virQEMUDriverGetConfig(driver); > + > + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) > + goto cleanup; > + > + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) > + goto endjob; > + > + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, > + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, groupname) < 0) > + goto endjob; > + > + if (qemuDomainSetBlockIoTuneFields(&info, > + params, > + nparams, > + &set_fields, > + &eventParams, > + &eventNparams, > + &eventMaxparams) < 0) > + goto endjob; > + > + if (qemuDomainCheckBlockIoTuneMutualExclusion(&info) < 0) > + goto endjob; > + > + virDomainThrottleGroupDefCopy(&info, &conf_info); > + > + priv = vm->privateData; > + qemuCaps = priv->qemuCaps; > + /* this throttle group feature requires "QEMU_CAPS_OBJECT_JSON" > + * when starting domain later, so check such flag here as well */ > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_JSON)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("QEMU_CAPS_OBJECT_JSON support is required for throttle group creation")); > + return -1; > + } If the VM is not alive the above check will not work. If the VM is not alive this must not be checked. Also _("QEMU_CAPS_OBJECT_JSON is an internal name and VIR_ERR_INTERNAL_ERROR is not appropriate.

> + > + if (def) { > + if (qemuDomainCheckThrottleGroupReset(groupname, &info) < 0) > + goto endjob; > + > + if (qemuDomainCheckBlockIoTuneMax(&info) < 0) > + goto endjob; > + > + cur_info = virDomainThrottleGroupByName(def, groupname); > + /* Update existing group. */ > + if (cur_info != NULL) { > + if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, set_fields) < 0) > + goto endjob; > + qemuDomainObjEnterMonitor(vm); > + rc = qemuMonitorUpdateThrottleGroup(qemuDomainGetMonitor(vm), > + groupname, > + &info); > + qemuDomainObjExitMonitor(vm); > + if (rc < 0) > + goto endjob; > + virDomainThrottleGroupUpdate(def, &info); > + } else { > + if (qemuMonitorThrottleGroupLimits(limits, &info)<0) > + goto endjob; > + if (qemuMonitorCreateObjectProps(&props, > + "throttle-group", groupname, As noted groupname must be prefixed when used with qemu to avoid namespacing issues by users being able to choose a name. > + "a:limits", &limits, > + NULL) < 0) > + goto endjob; > + qemuDomainObjEnterMonitor(vm); > + /* different QMP version has different format for "object-add", reuse > + * "qemuMonitorAddObject" to check if "objectAddNoWrap"("props") is > + * requried */ Regardless of this comment all object creation must use the appropriate API so this comment seems pointless. > + rc = qemuMonitorAddObject(qemuDomainGetMonitor(vm), &props, NULL); > + qemuDomainObjExitMonitor(vm); > + if (rc < 0) > + goto endjob; > + virDomainThrottleGroupAdd(def, &info); > + } > + > + qemuDomainSaveStatus(vm); > + > + if (eventNparams) { > + event = virDomainEventTunableNewFromDom(dom, &eventParams, eventNparams); > + virObjectEventStateQueue(driver->domainEventState, event); > + } > + } > + > + if (persistentDef) { > + conf_cur_info = virDomainThrottleGroupByName(persistentDef, groupname); > + > + if (qemuDomainCheckThrottleGroupReset(groupname, &conf_info) < 0) > + goto endjob; > + > + if (conf_cur_info != NULL) { > + if (qemuDomainSetBlockIoTuneDefaults(&conf_info, conf_cur_info, > + set_fields) < 0) > + goto endjob; > + virDomainThrottleGroupUpdate(persistentDef, &conf_info); > + } else { > + virDomainThrottleGroupAdd(persistentDef, &conf_info); > + } > + > + > + if (virDomainDefSave(persistentDef, driver->xmlopt, > + cfg->configDir) < 0) > + goto endjob; > + } > + > + ret = 0; > + endjob: > + virDomainObjEndJob(vm); > + > + cleanup: > + virDomainObjEndAPI(&vm); > + virTypedParamsFree(eventParams, eventNparams); > + return ret; > +} > + > + > +static int > +qemuDomainGetThrottleGroup(virDomainPtr dom, > + const char *groupname, > + virTypedParameterPtr *params, > + int *nparams, > + unsigned int flags) > +{ > + virDomainObj *vm = NULL; > + virDomainDef *def = NULL; > + virDomainDef *persistentDef = NULL; > + virDomainThrottleGroupDef groupDef = { 0 }; > + virDomainThrottleGroupDef *reply = &groupDef; > + int ret = -1; > + int maxparams = 0; > + int rc = 0; > + > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > + VIR_DOMAIN_AFFECT_CONFIG | > + VIR_TYPED_PARAM_STRING_OKAY, -1); As noted all callers which know this API are new enough to support typed param string so this flag doesn't make sense. > + > + > + if (!(vm = qemuDomainObjFromDomain(dom))) > + return -1; > + > + if (virDomainGetThrottleGroupEnsureACL(dom->conn, vm->def) < 0) > + goto cleanup; > + > + if (virDomainObjBeginJob(vm, VIR_JOB_QUERY) < 0) > + goto cleanup; > + > + /* the API check guarantees that only one of the definitions will be set */ > + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) > + goto endjob; I might have misplaced a comment about supporting mutual exclusivity in reply to previous patch. If that is so please ignore that comment. > + > + if (def) { > + qemuDomainObjEnterMonitor(vm); > + rc = qemuMonitorGetThrottleGroup(qemuDomainGetMonitor(vm), groupname, reply); You don't validate that the given throttle group even exists. You shouldn't allow passing that to qemu if it's not in the definition. Also as noted before you'll have to prefix the group name to avoid namespace collisions. > + qemuDomainObjExitMonitor(vm); > + > + if (rc < 0) > + goto endjob; > + } > + > + if (persistentDef) { > + reply = virDomainThrottleGroupByName(persistentDef, groupname); > + if (reply == NULL) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("throttle group '%1$s' was not found in the domain config"), > + groupname); > + goto endjob; > + } > + reply->group_name = g_strdup(groupname); > + } > + > + *nparams = 0; > + > +#define THROTTLE_GROUP_ASSIGN(name, var) \ > + if (virTypedParamsAddULLong(params, \ > + nparams, \ > + &maxparams, \ > + VIR_DOMAIN_BLOCK_IOTUNE_ ## name, \ > + reply->var) < 0) \ > + goto endjob; > + > + > + if (virTypedParamsAddString(params, nparams, &maxparams, > + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, > + reply->group_name) < 0) > + goto endjob; > + > + THROTTLE_GROUP_ASSIGN(TOTAL_BYTES_SEC, total_bytes_sec); > + THROTTLE_GROUP_ASSIGN(READ_BYTES_SEC, read_bytes_sec); > + THROTTLE_GROUP_ASSIGN(WRITE_BYTES_SEC, write_bytes_sec); > + > + THROTTLE_GROUP_ASSIGN(TOTAL_IOPS_SEC, total_iops_sec); > + THROTTLE_GROUP_ASSIGN(READ_IOPS_SEC, read_iops_sec); > + THROTTLE_GROUP_ASSIGN(WRITE_IOPS_SEC, write_iops_sec); > + > + THROTTLE_GROUP_ASSIGN(TOTAL_BYTES_SEC_MAX, total_bytes_sec_max); > + THROTTLE_GROUP_ASSIGN(READ_BYTES_SEC_MAX, read_bytes_sec_max); > + THROTTLE_GROUP_ASSIGN(WRITE_BYTES_SEC_MAX, write_bytes_sec_max); > + > + THROTTLE_GROUP_ASSIGN(TOTAL_IOPS_SEC_MAX, total_iops_sec_max); > + THROTTLE_GROUP_ASSIGN(READ_IOPS_SEC_MAX, read_iops_sec_max); > + THROTTLE_GROUP_ASSIGN(WRITE_IOPS_SEC_MAX, write_iops_sec_max); > + > + THROTTLE_GROUP_ASSIGN(SIZE_IOPS_SEC, size_iops_sec); > + > + THROTTLE_GROUP_ASSIGN(TOTAL_BYTES_SEC_MAX_LENGTH, total_bytes_sec_max_length); > + THROTTLE_GROUP_ASSIGN(READ_BYTES_SEC_MAX_LENGTH, read_bytes_sec_max_length); > + THROTTLE_GROUP_ASSIGN(WRITE_BYTES_SEC_MAX_LENGTH, write_bytes_sec_max_length); > + > + THROTTLE_GROUP_ASSIGN(TOTAL_IOPS_SEC_MAX_LENGTH, total_iops_sec_max_length); > + THROTTLE_GROUP_ASSIGN(READ_IOPS_SEC_MAX_LENGTH, read_iops_sec_max_length); > + THROTTLE_GROUP_ASSIGN(WRITE_IOPS_SEC_MAX_LENGTH, write_iops_sec_max_length); > +#undef THROTTLE_GROUP_ASSIGN > + > + ret = 0; > + > + endjob: > + virDomainObjEndJob(vm); > + > + cleanup: > + virDomainObjEndAPI(&vm); > + return ret; > +} > + > + > +static int > +qemuDomainCheckThrottleGroupRef(virDomainDef *def, > + const char *group_name) > +{ > + size_t i; > + for (i = 0; i < def->ndisks; i++) { > + virDomainDiskDef *disk = def->disks[i]; > + if (virDomainThrottleFilterFind(disk, group_name)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, This is not an internal error but invalid argument from the user. > + _("throttle group '%1$s' is still being used by disk %2$s"), > + group_name, disk->dst); > + return -1; > + } > + } > + return 0; > +} > + > + > +static int > +qemuDomainDelThrottleGroup(virDomainPtr dom, > + const char *groupname, > + unsigned int flags) > +{ > + virQEMUDriver *driver = dom->conn->privateData; > + virDomainObj *vm = NULL; > + virDomainDef *def = NULL; > + virDomainDef *persistentDef = NULL; > + g_autoptr(virQEMUDriverConfig) cfg = NULL; > + int ret = -1; > + > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > + VIR_DOMAIN_AFFECT_CONFIG | > + VIR_TYPED_PARAM_STRING_OKAY, -1); This API is not taking typed params. > + > + if (!(vm = qemuDomainObjFromDomain(dom))) > + return -1; > + > + if (virDomainDelThrottleGroupEnsureACL(dom->conn, vm->def) < 0) > + goto cleanup; > + > + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) > + goto cleanup; > + > + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) > + goto endjob; > + > + if (def) { > + int rc = 0; > + > + /* check if this group is still being used by disks */ > + if (qemuDomainCheckThrottleGroupRef(def, groupname) < 0) > + goto endjob; > + > + qemuDomainObjEnterMonitor(vm); > + rc = qemuMonitorDelObject(qemuDomainGetMonitor(vm), groupname, true); > + qemuDomainObjExitMonitor(vm); > + > + if (rc < 0) > + goto endjob; > + > + virDomainThrottleGroupDel(def, groupname); > + qemuDomainSaveStatus(vm); > + } > + > + if (persistentDef) { > + /* check if this group is still being used by disks */ > + if (qemuDomainCheckThrottleGroupRef(persistentDef, groupname) < 0) > + goto endjob; This should be done before the step modifying the live config so that you catch errors upfront and avoid a partial success with error reported. > + > + cfg = virQEMUDriverGetConfig(driver); > + virDomainThrottleGroupDel(persistentDef, groupname); > + if (virDomainDefSave(persistentDef, driver->xmlopt, > + cfg->configDir) < 0) > + goto endjob; > + } > + > + ret = 0; > + > + endjob: > + virDomainObjEndJob(vm); > + > + cleanup: > + virDomainObjEndAPI(&vm); > + return ret; > +}