[PATCH v4 4/7] migration/dirtyrate: Implement qemuDomainQueryDirtyRate

[Hao Wang]

Thanks for these reviews too. I'll apply all of them.

On 2020/11/11 4:11, Michal Privoznik wrote:
> On 11/7/20 10:41 AM, Hao Wang wrote:
>> Implement qemuDomainQueryDirtyRate which query domain's memory
>> dirty rate calling qmp "query-dirty-rate".
>>
>> Signed-off-by: Hao Wang <wanghao232 at huawei.com>
>> Signed-off-by: Zhou Yimin <zhouyimin at huawei.com>
>> Reviewed-by: Chuan Zheng <zhengchuan at huawei.com>
>> ---
>>   src/qemu/qemu_migration.c    | 31 +++++++++++++++++++++++++++++++
>>   src/qemu/qemu_migration.h    |  5 +++++
>>   src/qemu/qemu_monitor.c      | 12 ++++++++++++
>>   src/qemu/qemu_monitor.h      |  4 ++++
>>   src/qemu/qemu_monitor_json.c | 22 ++++++++++++++++++++++
>>   src/qemu/qemu_monitor_json.h |  4 ++++
>>   6 files changed, 78 insertions(+)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 8029e24415..3d07ba3ac4 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -5864,3 +5864,34 @@ qemuDomainCalculateDirtyRate(virDomainPtr dom,
>>         return ret;
>>   }
>> +
>> +
>> +int
>> +qemuDomainQueryDirtyRate(virDomainPtr dom,
>> +                         virDomainObjPtr vm,
>> +                         virDomainDirtyRateInfoPtr info)
>> +{
>> +    virQEMUDriverPtr driver = dom->conn->privateData;
> 
> Again, driver is accessible from the caller, just pass it directly.
> 
>> +    qemuDomainObjPrivatePtr priv;
>> +    int ret = -1;
>> +
>> +    if (!virDomainObjIsActive(vm)) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>> +                       "%s", _("domain is not running"));
>> +        return ret;
>> +    }
>> +
>> +    priv = vm->privateData;
>> +
>> +    qemuDomainObjEnterMonitor(driver, vm);
>> +
>> +    ret = qemuMonitorQueryDirtyRate(priv->mon, info);
>> +    if (ret < 0) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED,
>> +                       "%s", _("get vm's dirty rate failed."));
> 
> No, this is not correct, qemuMonitorQueryDirtyRate() will report an error if something goes wrong. And with pretty accurate error message. This overwrites it with more generic one.
> 
>> +    }
>> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> +        ret = -1;
>> +
>> +    return ret;
>> +}
>> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
>> index 0522b375c0..8baae512b7 100644
>> --- a/src/qemu/qemu_migration.h
>> +++ b/src/qemu/qemu_migration.h
>> @@ -263,3 +263,8 @@ int
>>   qemuDomainCalculateDirtyRate(virDomainPtr dom,
>>                                virDomainObjPtr vm,
>>                                long long sec);
>> +
>> +int
>> +qemuDomainQueryDirtyRate(virDomainPtr dom,
>> +                         virDomainObjPtr vm,
>> +                         virDomainDirtyRateInfoPtr info);
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 06603b8691..2fa6879467 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -4781,3 +4781,15 @@ qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon,
>>         return qemuMonitorJSONCalculateDirtyRate(mon, sec);
>>   }
>> +
>> +
>> +int
>> +qemuMonitorQueryDirtyRate(qemuMonitorPtr mon,
>> +                          virDomainDirtyRateInfoPtr info)
>> +{
>> +    VIR_DEBUG("info=%p", info);
>> +
>> +    QEMU_CHECK_MONITOR(mon);
>> +
>> +    return qemuMonitorJSONQueryDirtyRate(mon, info);
>> +}
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index afb97780cf..25105c3ad9 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -1531,3 +1531,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions,
>>   int
>>   qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon,
>>                                 long long sec);
>> +
>> +int
>> +qemuMonitorQueryDirtyRate(qemuMonitorPtr mon,
>> +                          virDomainDirtyRateInfoPtr info);
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index 65691522fb..1924c7229b 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -9657,3 +9657,25 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
>>         return 0;
>>   }
>> +
>> +
>> +int
>> +qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
>> +                              virDomainDirtyRateInfoPtr info)
>> +{
>> +    g_autoptr(virJSONValue) cmd = NULL;
>> +    g_autoptr(virJSONValue) reply = NULL;
>> +
>> +    if (!(cmd = qemuMonitorJSONMakeCommand("query-dirty-rate", NULL)))
>> +        return -1;
>> +
>> +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
>> +        return -1;
>> +
>> +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>> +        return -1;
>> +
>> +    /* TODO: extract dirtyrate data from reply and store in virDomainDirtyRateInfoPtr */
> 
> How about instead of noting down to finish this, squash 5/7 here? It's not like somebody could backport only this or that patch.
> 
>> +    info->status = 0;
>> +    return 0;
>> +}
>> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
>> index c9556fc19a..487f2e6e58 100644
>> --- a/src/qemu/qemu_monitor_json.h
>> +++ b/src/qemu/qemu_monitor_json.h
>> @@ -714,3 +714,7 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon,
>>   int
>>   qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
>>                                     long long sec);
>> +
>> +int
>> +qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
>> +                              virDomainDirtyRateInfoPtr info);
>>
> 
> I think the following should be squashed in:
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 5a253bbe2f..c0df2589da 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5864,29 +5864,21 @@ qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver,
> 
> 
>  int
> -qemuDomainQueryDirtyRate(virDomainPtr dom,
> +qemuDomainQueryDirtyRate(virQEMUDriverPtr driver,
>                           virDomainObjPtr vm,
>                           virDomainDirtyRateInfoPtr info)
>  {
> -    virQEMUDriverPtr driver = dom->conn->privateData;
> -    qemuDomainObjPrivatePtr priv;
> -    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int ret;
> 
>      if (!virDomainObjIsActive(vm)) {
>          virReportError(VIR_ERR_OPERATION_INVALID,
>                         "%s", _("domain is not running"));
> -        return ret;
> +        return -1;
>      }
> 
> -    priv = vm->privateData;
> -
>      qemuDomainObjEnterMonitor(driver, vm);
> -
>      ret = qemuMonitorQueryDirtyRate(priv->mon, info);
> -    if (ret < 0) {
> -        virReportError(VIR_ERR_OPERATION_FAILED,
> -                       "%s", _("get vm's dirty rate failed."));
> -    }
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          ret = -1;
> 
> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
> index f03bbaa3d9..c4be340ecb 100644
> --- a/src/qemu/qemu_migration.h
> +++ b/src/qemu/qemu_migration.h
> @@ -265,6 +265,6 @@ qemuDomainCalculateDirtyRate(virQEMUDriverPtr driver,
>                               long long sec);
> 
>  int
> -qemuDomainQueryDirtyRate(virDomainPtr dom,
> +qemuDomainQueryDirtyRate(virQEMUDriverPtr driver,
>                           virDomainObjPtr vm,
>                           virDomainDirtyRateInfoPtr info);
> 
> 
> Michal
> 
> .