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@xxxxxxxxxxxxx wrote:
> > > From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxx>
> > > ---
> > > 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.
