Re: [PATCH v5 4/5] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo

[Michal Privoznik]

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 
Reviewed-by: Chuan Zheng 
---
  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

[PATCH v5 4/5] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo

[Hao Wang]

Implement qemuDomainGetDirtyRateInfo: using flags to control behaviors
-- calculate and/or query dirtyrate. Default flag is both calculate
and query.

Signed-off-by: Hao Wang 
Reviewed-by: Chuan Zheng 
---
 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;
+
+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);
+}
+
+if (qemuDomainQueryDirtyRate(driver, vm, info) < 0)
+goto endjob;
+}
+
+ret = 0;
+
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+return ret;
+}
+
+
 static virHypervisorDriver qemuHypervisorDriver = {
 .name = QEMU_DRIVER_NAME,
 .connectURIProbe = qemuConnectURIProbe,
@@ -20530,6 +20596,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
 .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
 .domainAuthorizedSSHKeysGet = qemuDomainAuthorizedSSHKeysGet, /* 6.10.0 */
 .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */
+.domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 7.1.0 */
 };
 
 
-- 
2.23.0