Re: [PATCH v2 4/6] qemu_driver: Add mode option for qemuDomainStartDirtyRateCalc

On Thu, Jan 27, 2022 at 15:25:20 +0800, huangy81(a)chinatelecom.cn wrote: > From: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn&gt; > > Add mode option to extend qemuDomainStartDirtyRateCalc API, > which is introduced since qemu >= 6.2. > > Signed-off-by: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn&gt; > --- > include/libvirt/libvirt-domain.h | 13 +++++++++++++ > src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++++-- > src/qemu/qemu_monitor.c | 5 +++-- > src/qemu/qemu_monitor.h | 10 +++++++++- > src/qemu/qemu_monitor_json.c | 26 +++++++++++++++++++++----- > src/qemu/qemu_monitor_json.h | 3 ++- > 6 files changed, 79 insertions(+), 11 deletions(-) > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > index 4da1a63..54bb23b 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -5257,6 +5257,19 @@ typedef enum { > # endif > } virDomainDirtyRateStatus; > > +/** > + * virDomainDirtyRateCalcFlags: > + * > + * Flags OR'ed together to provide specific behaviour when calculating dirty page > + * rate for a Domain > + * > + */ > +typedef enum { > + VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING = 0, /* default mode - page-sampling */ > + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP = 1 << 0, /* dirty-bitmap mode */ > + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING = 1 << 1, /* dirty-ring mode */ > +} virDomainDirtyRateCalcFlags; > + > int virDomainStartDirtyRateCalc(virDomainPtr domain, > int seconds, > unsigned int flags); > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 0e8e9b1..feebfc4 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -20648,9 +20648,13 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom, > virDomainObj *vm = NULL; > qemuDomainObjPrivate *priv; > g_autoptr(virQEMUCaps) qemucaps = NULL; > + qemuMonitorDirtyRateCalcMode calcmode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_PAGE_SAMPLING; > + bool mode = false; > int ret = -1; > > - virCheckFlags(0, -1); > + virCheckFlags(VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING | > + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP | > + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING, -1); > > if (!(qemucaps = virQEMUCapsCacheLookupDefault(driver->qemuCapsCache, > NULL, NULL, NULL, NULL, > @@ -20663,6 +20667,15 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom, > return -1; > } > > + mode = virQEMUCapsGet(qemucaps, QEMU_CAPS_DIRTYRATE_MODE); Same issue as I've pointed out previously, you must use priv->qemuCaps.

Also you can then merge the two blocks which are guarded by 'mode' and get rid of the extra variable. > + > + if (!mode && (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP || > + (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING))) { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > + _("QEMU does not support calculating dirty page rate" > + "with specified mode")); Here you are missing a goto cleanup or similar jump-out statement. Also please observe error messages in new code should not be line-broken even if the line will exceed 80 columns: https://www.libvirt.org/coding-style.html#error-message-format > + } > + > if (seconds < MIN_DIRTYRATE_CALC_PERIOD || > seconds > MAX_DIRTYRATE_CALC_PERIOD) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > @@ -20676,6 +20689,22 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom, > if (!(vm = qemuDomainObjFromDomain(dom))) > return -1; > > + if (mode) { > + /* libvirt-domain.c already guaranteed these two flags are exclusive. */ There isn't any code being added which would guarantee that, so the comment is either to be removed or something's missing in this commit. > + if (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP) { > + calcmode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_DIRTY_BITMAP; > + } else if (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING) { > + if (vm->def->features[VIR_DOMAIN_FEATURE_KVM] != VIR_TRISTATE_SWITCH_ON || > + vm->def->kvm_features->features[VIR_DOMAIN_KVM_DIRTY_RING] != VIR_TRISTATE_SWITCH_ON) { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", All of this code must be moved below the ACL check or otherwise it could leak information about the VM object to users unauthorized to even see the VM object. > + _("Calculating dirty page rate with dirty-ring requires" > + "dirty-ring feature enabled.")); > + goto cleanup; > + } > + calcmode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_DIRTY_RING; > + } > + } > + > if (virDomainStartDirtyRateCalcEnsureACL(dom->conn, vm->def) < 0) > goto cleanup; > > @@ -20692,7 +20721,7 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom, > > priv = vm->privateData; > qemuDomainObjEnterMonitor(driver, vm); > - ret = qemuMonitorStartDirtyRateCalc(priv->mon, seconds); > + ret = qemuMonitorStartDirtyRateCalc(priv->mon, seconds, calcmode); > > qemuDomainObjExitMonitor(driver, vm); > [...] > struct _qemuMonitorDirtyRateInfo { > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index b0b5136..afbd721 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -8695,18 +8695,34 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitor *mon, > migratable); > } > > +VIR_ENUM_DECL(qemuMonitorDirtyRateCalcMode); > +VIR_ENUM_IMPL(qemuMonitorDirtyRateCalcMode, > + VIR_DOMAIN_DIRTYRATE_CALC_MODE_LAST, > + "page-sampling", > + "dirty-bitmap", > + "dirty-ring"); > > int > qemuMonitorJSONStartDirtyRateCalc(qemuMonitor *mon, > - int seconds) > + int seconds, > + qemuMonitorDirtyRateCalcMode mode) > { > g_autoptr(virJSONValue) cmd = NULL; > g_autoptr(virJSONValue) reply = NULL; > > - if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate", > - "i:calc-time", seconds, > - NULL))) > - return -1; > + if (mode == VIR_DOMAIN_DIRTYRATE_CALC_MODE_PAGE_SAMPLING) { > + if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate", > + "i:calc-time", seconds, > + NULL))) > + return -1; > + } else { > + const char *modestr = qemuMonitorDirtyRateCalcModeTypeToString(mode); > + if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate", > + "i:calc-time", seconds, > + "s:mode", modestr, > + NULL))) > + return -1; > + } You don't have to use two invocations of qemuMonitorJSONMakeCommand, but rather use 'S' for formatting the string which doesn't output the field if the string is null, so the code should look like: const char *modestr = NULL; if (mode != VIR_DOMAIN_DIRTYRATE_CALC_MODE_PAGE_SAMPLING) modestr = qemuMonitorDirtyRateCalcModeTypeToString(mode); if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate", "i:calc-time", seconds, "S:mode", modestr, NULL)))