On 2/1/21 10:55 AM, Hao Wang wrote:
Implement qemuDomainGetDirtyRateInfo: using flags to control behaviors
-- calculate and/or query dirtyrate. Default flag is both calculate
and query.

Signed-off-by: Hao Wang <wanghao...@huawei.com>
Reviewed-by: Chuan Zheng <zhengch...@huawei.com>
---
  src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 67 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ed840a5c8d..2b9ce1c386 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20289,6 +20289,72 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
  }
+#define MIN_DIRTYRATE_CALCULATION_PERIOD 1 /* supported min dirtyrate calc time: 1s */
+#define MAX_DIRTYRATE_CALCULATION_PERIOD 60 /* supported max dirtyrate calc 
time: 60s */
+
+static int
+qemuDomainGetDirtyRateInfo(virDomainPtr dom,
+                           virDomainDirtyRateInfoPtr info,
+                           int sec,
+                           unsigned int flags)
+{
+    virQEMUDriverPtr driver = dom->conn->privateData;
+    virDomainObjPtr vm = NULL;
+    int ret = -1;


Here should be virCheckFlags() call with all flags supported enumarated. The idea is that if a new flag is ever invented then instead of ignoring it silently an error is produced.

+
+    if (!(vm = qemuDomainObjFromDomain(dom)))
+        return -1;
+
+    if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0)
+        goto cleanup;
+
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+        goto cleanup;
+
+    /* flags is default to both calculate and query */
+    if (flags == VIR_DOMAIN_DIRTYRATE_DEFAULT)
+        flags |= VIR_DOMAIN_DIRTYRATE_CALC | VIR_DOMAIN_DIRTYRATE_QUERY;
+
+    /* calculating */
+    if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
+        if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD ||
+            sec > MAX_DIRTYRATE_CALCULATION_PERIOD) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("seconds=%d is invalid, please choose value within 
[%d, %d]."),
+                           sec,
+                           MIN_DIRTYRATE_CALCULATION_PERIOD,
+                           MAX_DIRTYRATE_CALCULATION_PERIOD);
+            goto endjob;
+        }
+
+        if (qemuDomainCalculateDirtyRate(driver, vm, sec) < 0)
+            goto endjob;
+    }
+
+    /* querying */
+    if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) {
+        /* wait sec and extra 50ms to let last calculation finish */
+        if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
+            virObjectUnlock(vm);
+            g_usleep((sec * 1000 + 50) * 1000);
+            virObjectLock(vm);

I know this will probably change, but anyway - waiting X seconds is not good IMO. Also, it performs two operations at once. What if the calculation was successfully started but then querying fails? How can a caller distinguish this from a case where the calculation failed to start?

Michal

Reply via email to