[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 <wanghao232 at huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan at 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