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, [email protected] wrote:
> > > From: Chun Feng Wu <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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.
