6:20 a.m.
Thanks for the reviews.
On 2020/11/11 4:11, Michal Privoznik wrote:
On 11/7/20 10:41 AM, Hao Wang wrote:
> Implement qemuDomainGetDirtyRateInfo:
> using flags to control behaviors -- calculate and/or query dirtyrate.
>
> Signed-off-by: Hao Wang <wanghao232(a)huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan(a)huawei.com>
> ---
> include/libvirt/libvirt-domain.h | 11 ++++++
> src/qemu/qemu_driver.c | 68 ++++++++++++++++++++++++++++++++
> 2 files changed, 79 insertions(+)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 51d8685086..fc45f42dcf 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -5096,6 +5096,17 @@ int virDomainBackupBegin(virDomainPtr domain,
> char *virDomainBackupGetXMLDesc(virDomainPtr domain,
> unsigned int flags);
> +/**
> + * virDomainDirtyRateFlags:
> + *
> + * Details on the flags used by getdirtyrate api.
> + */
> +
> +typedef enum {
> + VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirtyrate
*/
> + VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */
> +} virDomainDirtyRateFlags;
> +
> /**
> * virDomainDirtyRateStatus:
> *
Again, doesn't belong here.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cb56fbbfcf..93d5a23630 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20121,6 +20121,73 @@ qemuDomainAgentSetResponseTimeout(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,
> + long long sec,
> + unsigned int flags)
> +{
> + virDomainObjPtr vm = NULL;
> + virQEMUDriverPtr driver = dom->conn->privateData;
> + int ret = -1;
> +
> + if (!(vm = qemuDomainObjFromDomain(dom)))
> + return ret;
> +
> + if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> + goto cleanup;
> +
> + if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0))
> + goto endjob;
> +
Why is this check needed? I don't understand it, can you please explain?
It's indeed not necessary. I'll remove it.
> + if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
> + if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || sec >
MAX_DIRTYRATE_CALCULATION_PERIOD) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("seconds=%lld is invalid, please choose value
within [1, 60]."),
> + sec);
> + goto endjob;
> + }
> +
> + if (qemuDomainCalculateDirtyRate(dom, vm, sec) < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("can't calculate domain's dirty
rate"));
This overwrites a more accurate error reported by qemuDomainCalculateDirtyRate().
> + goto endjob;
> + }
> + }
> +
> + if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) {
> + if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
> + struct timespec ts = { .tv_sec = sec, .tv_nsec = 50 * 1000 * 1000ull };
> +
> + virObjectUnlock(vm);
> + nanosleep(&ts, NULL);
> + virObjectLock(vm);
At first I was afraid, I was petrified that this waits for 50 seconds, until I realized
it's nanosleep(). Perhaps g_usleep(50*1000); would look better?
> + }
> +
> + if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("can't query domain's dirty rate"));
Again, error overwrite.
> + goto endjob;
> + }
> + }
So if no flag is specified then nothing happens? I know you handle that in virsh, but I
think that logic should live here. And perhaps the public API description should document
that no flags is equal to specifying both flags together.
> +
> + ret = 0;
> +
> + endjob:
> + qemuDomainObjEndJob(driver, vm);
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
> +
> static virHypervisorDriver qemuHypervisorDriver = {
> .name = QEMU_DRIVER_NAME,
> .connectURIProbe = qemuConnectURIProbe,
> @@ -20360,6 +20427,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
> .domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /* 5.10.0
*/
> .domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */
> .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
> + .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.9.0 */
6.10.0 :-(
> };
>
I think the following should be squashed in:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4cbbe8dd84..4058fb487c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20130,12 +20130,12 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,
long long sec,
unsigned int flags)
{
- virDomainObjPtr vm = NULL;
virQEMUDriverPtr driver = dom->conn->privateData;
+ virDomainObjPtr vm = NULL;
int ret = -1;
if (!(vm = qemuDomainObjFromDomain(dom)))
- return ret;
+ return -1;
if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
@@ -20147,18 +20147,18 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,
goto endjob;
if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
- if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || sec >
MAX_DIRTYRATE_CALCULATION_PERIOD) {
+ if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD ||
+ sec > MAX_DIRTYRATE_CALCULATION_PERIOD) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("seconds=%lld is invalid, please choose value within
[1, 60]."),
- sec);
+ _("seconds=%lld is invalid, please choose value within
[%d, %d]."),
+ sec,
+ MIN_DIRTYRATE_CALCULATION_PERIOD,
+ MAX_DIRTYRATE_CALCULATION_PERIOD);
goto endjob;
}
- if (qemuDomainCalculateDirtyRate(dom, vm, sec) < 0) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("can't calculate domain's dirty rate"));
+ if (qemuDomainCalculateDirtyRate(driver, vm, sec) < 0)
goto endjob;
- }
}
if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) {
@@ -20170,11 +20170,8 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,
virObjectLock(vm);
}
- if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("can't query domain's dirty rate"));
+ if (qemuDomainQueryDirtyRate(driver, vm, info) < 0)
goto endjob;
- }
}
ret = 0;
@@ -20427,7 +20424,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /* 5.10.0 */
.domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */
.domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
- .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.9.0 */
+ .domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.10.0 */
};
Michal
.