I'll take these reviews in my next version. Thanks again!
On 2020/11/11 4:11, Michal Privoznik wrote:
> On 11/7/20 10:41 AM, Hao Wang wrote:
>> Implement qemuMonitorJSONExtractDirtyRateInfo to deal with the return from
>> qmp "query-dirty-rate", and store them in virDomainDirtyRateInfo.
>>
>> Signed-off-by: Hao Wang <wanghao232@xxxxxxxxxx>
>> Reviewed-by: Chuan Zheng <zhengchuan@xxxxxxxxxx>
>> ---
>> include/libvirt/libvirt-domain.h | 17 +++++++++
>> src/qemu/qemu_monitor_json.c | 59 ++++++++++++++++++++++++++++++--
>> 2 files changed, 73 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
>> index b950736b67..51d8685086 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -5096,6 +5096,23 @@ int virDomainBackupBegin(virDomainPtr domain,
>> char *virDomainBackupGetXMLDesc(virDomainPtr domain,
>> unsigned int flags);
>> +/**
>> + * virDomainDirtyRateStatus:
>> + *
>> + * Details on the cause of a dirtyrate calculation status.
>> + */
>> +
>> +typedef enum {
>> + VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has not been started */
>> + VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is measuring */
>> + VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is
>> +completed */
>> +
>> +# ifdef VIR_ENUM_SENTINELS
>> + VIR_DOMAIN_DIRTYRATE_LAST
>> +# endif
>> +} virDomainDirtyRateStatus;
>> +
>
> As advertised earlier, this doesn't belong into this commit really.
>
>> /**
>> * virDomainDirtyRateInfo:
>> *
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index 1924c7229b..ca7d8d23c0 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -9659,12 +9659,61 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
>> }
>> +VIR_ENUM_DECL(qemuDomainDirtyRateStatus);
>> +VIR_ENUM_IMPL(qemuDomainDirtyRateStatus,
>> + VIR_DOMAIN_DIRTYRATE_LAST,
>> + "unstarted",
>> + "measuring",
>> + "measured");
>> +
>> +static int
>> +qemuMonitorJSONExtractDirtyRateInfo(virJSONValuePtr data,
>> + virDomainDirtyRateInfoPtr info)
>> +{
>> + const char *status;
>> + int statusID;
>> +
>> + if (!(status = virJSONValueObjectGetString(data, "status"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("query-dirty-rate reply was missing 'status' data"));
>> + return -1;
>> + }
>> +
>> + if ((statusID = qemuDomainDirtyRateStatusTypeFromString(status)) < 0) {
>> + return -1;
>
> So if qemu sends us some other string, this fails silently.
>
>> + }
>> + info->status = statusID;
>> +
>> + if ((info->status == VIR_DOMAIN_DIRTYRATE_MEASURED) &&
>> + (virJSONValueObjectGetNumberLong(data, "dirty-rate", &(info->dirtyRate)) < 0)) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("query-dirty-rate reply was missing 'dirty-rate' data"));
>> + return -1;
>> + }
>> +
>> + if (virJSONValueObjectGetNumberLong(data, "start-time", &(info->startTime)) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("query-dirty-rate reply was missing 'start-time' data"));
>> + return -1;
>> + }
>> +
>> + if (virJSONValueObjectGetNumberLong(data, "calc-time", &(info->calcTime)) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("query-dirty-rate reply was missing 'calc-time' data"));
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> int
>> qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
>> virDomainDirtyRateInfoPtr info)
>> {
>> g_autoptr(virJSONValue) cmd = NULL;
>> g_autoptr(virJSONValue) reply = NULL;
>> + virJSONValuePtr data = NULL;
>> if (!(cmd = qemuMonitorJSONMakeCommand("query-dirty-rate", NULL)))
>> return -1;
>> @@ -9675,7 +9724,11 @@ qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
>> if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>> return -1;
>> - /* TODO: extract dirtyrate data from reply and store in virDomainDirtyRateInfoPtr */
>> - info->status = 0;
>> - return 0;
>> + if (!(data = virJSONValueObjectGetObject(reply, "return"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("query-dirty-rate reply was missing 'return' data"));
>> + return -1;
>> + }
>> +
>> + return qemuMonitorJSONExtractDirtyRateInfo(data, info);
>> }
>>
>
> I think the following should be squashed in:
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 78ad10bc24..4715b33254 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -9680,24 +9680,26 @@ qemuMonitorJSONExtractDirtyRateInfo(virJSONValuePtr data,
> }
>
> if ((statusID = qemuDomainDirtyRateStatusTypeFromString(status)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown dirty rate status: %s"), status);
> return -1;
> }
> info->status = statusID;
>
> if ((info->status == VIR_DOMAIN_DIRTYRATE_MEASURED) &&
> - (virJSONValueObjectGetNumberLong(data, "dirty-rate", &(info->dirtyRate)) < 0)) {
> + (virJSONValueObjectGetNumberLong(data, "dirty-rate", &info->dirtyRate) < 0)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("query-dirty-rate reply was missing 'dirty-rate' data"));
> return -1;
> }
>
> - if (virJSONValueObjectGetNumberLong(data, "start-time", &(info->startTime)) < 0) {
> + if (virJSONValueObjectGetNumberLong(data, "start-time", &info->startTime) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("query-dirty-rate reply was missing 'start-time' data"));
> return -1;
> }
>
> - if (virJSONValueObjectGetNumberLong(data, "calc-time", &(info->calcTime)) < 0) {
> + if (virJSONValueObjectGetNumberLong(data, "calc-time", &info->calcTime) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("query-dirty-rate reply was missing 'calc-time' data"));
> return -1;
>
>
> Michal
>
> .
