On Fri, Aug 09, 2024 at 16:00:11 +0800, Chun Feng Wu wrote:
On 2024/7/26 23:06, Peter Krempa wrote:
On Wed, Jun 12, 2024 at 03:02:16 -0700, wucf@linux.ibm.com wrote:
From: Chun Feng Wu <wucf@linux.ibm.com>
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@linux.ibm.com> --- src/qemu/qemu_driver.c | 611 +++++++++++++++++++++++++++++++++++------ 1 file changed, 528 insertions(+), 83 deletions(-)
[...]
+ + 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.
is it okay to update it as:
if (virDomainObjIsActive(vm)) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_JSON)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("support for '-object' with json format in QEMU command is required when creating throttle group"));
"This QEMU doesn't support throttle group creation" The detail about needing JSON for '-object' is not really necessary in the error message.
} }
BTW, may I know if you finished all commits review for v3?
Not yet. I need to get back to it. Sorry I was busy.