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@xxxxxxxxxx>
>> Signed-off-by: Zhou Yimin <zhouyimin@xxxxxxxxxx>
>> Reviewed-by: Chuan Zheng <zhengchuan@xxxxxxxxxx>
>> ---
>> 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
>
> .
