On Thu, Apr 11, 2024 at 19:01:52 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> Implement the following methods:
> * virDomainSetThrottleGroup
> * virDomainGetThrottleGroup
> * virDomainDelThrottleGroup
Similarly to previous patch, note how you've done this rather than what
you've done, which is visible from the diff.
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
As noted in previous patch this really needs to go after the XML and
qemu commandline bits.
> src/qemu/qemu_domain.c | 14 ++
> src/qemu/qemu_domain.h | 4 +
> src/qemu/qemu_driver.c | 523 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 541 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6b869797a8..b676f59c3a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10246,6 +10246,20 @@ qemuDomainDiskByName(virDomainDef *def,
> }
>
>
> +virDomainThrottleGroupDef *
> +qemuDomainThrottleGroupByName(virDomainDef *def,
> + const char *name)
> +{
> + virDomainThrottleGroupDef *ret;
> +
> + if (!(ret = virDomainThrottleGroupByName(def, name))) {
> + return NULL;
This wrapper is really pointless. Use virDomainThrottleGroupByName
directly.
> + }
> +
> + return ret;
> +}
> +
> +
> int
> qemuDomainPrepareChannel(virDomainChrDef *channel,
> const char *domainChannelTargetDir)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d01f788aea..da0f9590db 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19995,6 +19995,526 @@ qemuDomainGraphicsReload(virDomainPtr domain,
> return ret;
> }
>
> +
> +/* wrapper of qemuDomainSetBlockIoTuneDefaults for throttle group since they use the same data structure */
> +static int
> +qemuDomainSetThrottleGroupDefaults(virDomainBlockIoTuneInfo *newinfo,
> + virDomainBlockIoTuneInfo *oldinfo,
> + qemuBlockIoTuneSetFlags set_fields)
> +{
> + return qemuDomainSetBlockIoTuneDefaults(newinfo, oldinfo, set_fields);
> +}
Same here, this is a pointless wrapper.
> +
> +
> +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);
It's not quite obvious what this function is supposed to do and on the
first glance it looks like it's a set of pre-checks. As of such you'll
be better of doing this set of checks
> +
> + 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;
> + size_t i;
> + 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();
> +
> +
> + 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,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
> + VIR_TYPED_PARAM_STRING,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + NULL) < 0)
> + return -1;
> +
> + 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_THROTTLE_GROUP, groupname) < 0)
> + goto endjob;
So what if the string is already there? It is allowed above.
> +
> +#define SET_THROTTLE_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, \
> + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \
> + param->value.ul) < 0) \
> + goto endjob; \
> + continue; \
> + }
> +
> + for (i = 0; i < nparams; i++) {
> + virTypedParameterPtr param = ¶ms[i];
> +
> + if (param->value.ul > QEMU_BLOCK_IOTUNE_MAX) {
> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> + _("throttle group value must be no more than %1$llu"),
> + QEMU_BLOCK_IOTUNE_MAX);
> + goto endjob;
> + }
> +
> + SET_THROTTLE_FIELD(total_bytes_sec, BYTES, TOTAL_BYTES_SEC);
> + SET_THROTTLE_FIELD(read_bytes_sec, BYTES, READ_BYTES_SEC);
> + SET_THROTTLE_FIELD(write_bytes_sec, BYTES, WRITE_BYTES_SEC);
> + SET_THROTTLE_FIELD(total_iops_sec, IOPS, TOTAL_IOPS_SEC);
> + SET_THROTTLE_FIELD(read_iops_sec, IOPS, READ_IOPS_SEC);
> + SET_THROTTLE_FIELD(write_iops_sec, IOPS, WRITE_IOPS_SEC);
> +
> + SET_THROTTLE_FIELD(total_bytes_sec_max, BYTES_MAX,
> + TOTAL_BYTES_SEC_MAX);
> + SET_THROTTLE_FIELD(read_bytes_sec_max, BYTES_MAX,
> + READ_BYTES_SEC_MAX);
> + SET_THROTTLE_FIELD(write_bytes_sec_max, BYTES_MAX,
> + WRITE_BYTES_SEC_MAX);
> + SET_THROTTLE_FIELD(total_iops_sec_max, IOPS_MAX,
> + TOTAL_IOPS_SEC_MAX);
> + SET_THROTTLE_FIELD(read_iops_sec_max, IOPS_MAX,
> + READ_IOPS_SEC_MAX);
> + SET_THROTTLE_FIELD(write_iops_sec_max, IOPS_MAX,
> + WRITE_IOPS_SEC_MAX);
> + SET_THROTTLE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC);
> +
> + 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,
> + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME,
> + param->value.s) < 0)
> + goto endjob;
> + continue;
> + }
> +
> + SET_THROTTLE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH,
> + TOTAL_BYTES_SEC_MAX_LENGTH);
> + SET_THROTTLE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH,
> + READ_BYTES_SEC_MAX_LENGTH);
> + SET_THROTTLE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH,
> + WRITE_BYTES_SEC_MAX_LENGTH);
> + SET_THROTTLE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH,
> + TOTAL_IOPS_SEC_MAX_LENGTH);
> + SET_THROTTLE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH,
> + READ_IOPS_SEC_MAX_LENGTH);
> + SET_THROTTLE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH,
> + WRITE_IOPS_SEC_MAX_LENGTH);
> + }
> +
> +#undef SET_THROTTLE_FIELD
> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + virDomainThrottleGroupDefCopy(&info, &conf_info);
> +
> + if (def) {
> + if (qemuDomainCheckThrottleGroupReset(groupname, &info) < 0)
> + goto endjob;
> +
> +#define CHECK_MAX(val, _bool) \
> + do { \
> + 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"), \
> + #val, #val "_max"); \
> + } else { \
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \
> + _("value '%1$s' cannot be set if '%2$s' is not set"), \
> + #val "_max", #val); \
> + } \
> + goto endjob; \
> + } \
> + if (info.val##_max < info.val) { \
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \
> + _("value '%1$s' cannot be smaller than '%2$s'"), \
> + #val "_max", #val); \
> + goto endjob; \
> + } \
> + } \
> + } while (false)
> +
> + CHECK_MAX(total_bytes_sec, BYTES);
> + CHECK_MAX(read_bytes_sec, BYTES);
> + CHECK_MAX(write_bytes_sec, BYTES);
> + CHECK_MAX(total_iops_sec, IOPS);
> + CHECK_MAX(read_iops_sec, IOPS);
> + CHECK_MAX(write_iops_sec, IOPS);
All of the above checks are duplicated from qemuDomainSetBlockIoTune.
Refactor them out first and reuse them rather than copy&paste.
> +
> +#undef CHECK_MAX
> +
> + cur_info = qemuDomainThrottleGroupByName(def, groupname);
> + /* Update existing group. */
> + if (cur_info != NULL) {
> + if (qemuDomainSetThrottleGroupDefaults(&info, cur_info,
> + set_fields) < 0)
Spacing is off here ..
> + goto endjob;
> + qemuDomainObjEnterMonitor(vm);
> + rc = qemuMonitorUpdateThrottleGroup(qemuDomainGetMonitor(vm),
> + groupname,
> + &info);
... here ...
> + qemuDomainObjExitMonitor(vm);
> + if (rc < 0)
> + goto endjob;
> + virDomainThrottleGroupUpdate(def, &info);
> + }else{
... here ...
> + if (qemuMonitorThrottleGroupLimits(limits, &info)<0)
> + goto endjob;
> + if (qemuMonitorCreateObjectProps(&props,
> + "throttle-group", groupname,
> + "a:limits", &limits,
... here ...
> + NULL) < 0)
> + goto endjob;
> + qemuDomainObjEnterMonitor(vm);
> + 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 = qemuDomainThrottleGroupByName(persistentDef, groupname);
> +
> + if (qemuDomainCheckThrottleGroupReset(groupname, &conf_info) < 0)
> + goto endjob;
> +
> + if (conf_cur_info != NULL) {
> + if (qemuDomainSetThrottleGroupDefaults(&conf_info, conf_cur_info,
> + set_fields) < 0)
... here ...
> + goto endjob;
> + virDomainThrottleGroupUpdate(persistentDef, &conf_info);
> + }else{
... here ...
> + 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;
> + int rc = 0;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG |
> + VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> + /* We don't return strings, and thus trivially support this flag. */
This is not true as ... [1]
> + flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> +
> + 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;
> +
> + maxparams = QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS;
> +
> + if (*nparams == 0) {
> + *nparams = maxparams;
> + ret = 0;
> + goto endjob;
> + }
> + if (*nparams < maxparams)
> + maxparams = *nparams;
All of this won't make sense once you return the array fully allocated.
> +
> + if (def) {
> + qemuDomainObjEnterMonitor(vm);
> + rc = qemuMonitorGetThrottleGroup(qemuDomainGetMonitor(vm), groupname, reply);
> + qemuDomainObjExitMonitor(vm);
> +
> + if (rc < 0)
> + goto endjob;
> + }
> +
> + if (persistentDef) {
> + reply = qemuDomainThrottleGroupByName(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 (*nparams < maxparams && \
> + virTypedParameterAssign(¶ms[(*nparams)++], \
> + VIR_DOMAIN_BLOCK_IOTUNE_ ## name, \
> + VIR_TYPED_PARAM_ULLONG, \
> + reply->var) < 0) \
> + goto endjob;
> +
> + if (*nparams < maxparams) {
> + if (virTypedParameterAssign(¶ms[(*nparams)++],
> + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
> + VIR_TYPED_PARAM_STRING,
[1].... you do in fact return a string.
> + 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
> +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);
> +
> + /* We don't return strings, and thus trivially support this flag. */
> + flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
This function doesn't even accept typed parameters, thus this really
makes no sense here.
> +
> + if (!(vm = qemuDomainObjFromDomain(dom)))
> + return -1;
> +
> + cfg = virQEMUDriverGetConfig(driver);
> +
> + if (virDomainDelThrottleGroupEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if (virDomainObjBeginJob(vm, VIR_JOB_QUERY) < 0)
> + goto cleanup;
Deleting stuff from the definition is definitely _not_ a VIR_JOB_QUERY.
> +
> + /* the API check guarantees that only one of the definitions will be set */
Umm so while this comment is true I don't think the deletion API should
refuse the combination of VIR_DOMAIN_AFFECT_LIVE and
VIR_DOMAIN_AFFECT_CONFIG.
While I agree that the combination of the flags can cause problems all
other APIs support this combination.
> + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
> + goto endjob;
This is also missing some form of checks that there are no longer any
disks which would make use of the throttling objects any more. I presume
that the monitor command will fail in such case.
> +
> + if (def) {
> + int rc = 0;
> +
> + qemuDomainObjEnterMonitor(vm);
> + rc = qemuMonitorDelObject(qemuDomainGetMonitor(vm), groupname, true);
> + qemuDomainObjExitMonitor(vm);
> +
> + if (rc < 0)
> + goto endjob;
> +
> + virDomainThrottleGroupDel(def, groupname);
> + qemuDomainSaveStatus(vm);
> + }
> +
> + if (persistentDef) {
You can fetch 'cfg' only in this block.
> + virDomainThrottleGroupDel(persistentDef, groupname);
> + if (virDomainDefSave(persistentDef, driver->xmlopt,
> + cfg->configDir) < 0)
> + goto endjob;
> + }
> +
> + ret = 0;
> +
> + endjob:
> + virDomainObjEndJob(vm);
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
