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

On 2024/7/26 23:06, Peter Krempa wrote: > 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(-)

> > + > > + 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"));

        } } BTW, may I know if you finished all commits review for v3?