Re: [PATCH v4 5/7] migration/dirtyrate: Implement qemuMonitorJSONExtractDirtyRateInfo

[Hao Wang]

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 
>> Reviewed-by: Chuan Zheng 
>> ---
>>   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
> @@ -968

Re: [PATCH v4 5/7] migration/dirtyrate: Implement qemuMonitorJSONExtractDirtyRateInfo

[Michal Privoznik]

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 
Reviewed-by: Chuan Zheng 
---
  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)) {
+

[PATCH v4 5/7] migration/dirtyrate: Implement qemuMonitorJSONExtractDirtyRateInfo

[Hao Wang]

Implement qemuMonitorJSONExtractDirtyRateInfo to deal with the return from
qmp "query-dirty-rate", and store them in virDomainDirtyRateInfo.

Signed-off-by: Hao Wang 
Reviewed-by: Chuan Zheng 
---
 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;
+
 /**
  * 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;
+}
+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);
 }
-- 
2.23.0