[PATCH v5 1/9] qemu: Refactor dirty page rate calculation status implementation

[Michal Prívozník]

On 2/16/22 01:28, huangy81 at chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
> 
> For any virTypedParameter API normal practice is to use a string
> to expose the data, not the rather enum integer value.
> 
> So let's drop the virDomainDirtyRateStatus in public header file
> and introduce internal enum def qemuMonitorDirtyRateStatus to
> describe the dirty page rate calculation status.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
> ---
>  include/libvirt/libvirt-domain.h | 18 ------------------
>  src/libvirt-domain.c             |  4 ++--
>  src/qemu/qemu_driver.c           |  9 ++++++---
>  src/qemu/qemu_monitor.h          | 18 ++++++++++++++++--
>  src/qemu/qemu_monitor_json.c     |  4 ++--
>  5 files changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 8c16598..bf4746a 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -5241,24 +5241,6 @@ int virDomainGetMessages(virDomainPtr domain,
>                           char ***msgs,
>                           unsigned int flags);
>  
> -/**
> - * virDomainDirtyRateStatus:
> - *
> - * Details on the cause of a dirty rate 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;
> -
>  int virDomainStartDirtyRateCalc(virDomainPtr domain,
>                                  int seconds,
>                                  unsigned int flags);
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index b8a6f10..f24d072 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11947,8 +11947,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   *     this format:
>   *
>   *     "dirtyrate.calc_status" - the status of last memory dirty rate calculation,
> - *                               returned as int from virDomainDirtyRateStatus
> - *                               enum.
> + *                               either of these 3 'unstarted,measuring,measured'
> + *                               values returned.
>   *     "dirtyrate.calc_start_time" - the start time of last memory dirty rate
>   *                                   calculation as long long.
>   *     "dirtyrate.calc_period" - the period of last memory dirty rate calculation
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f262020..a22646c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18525,6 +18525,8 @@ qemuDomainGetStatsDirtyRateMon(virQEMUDriver *driver,
>      return ret;
>  }
>  
> +VIR_ENUM_DECL(qemuMonitorDirtyRateStatus);
> +
>  static int
>  qemuDomainGetStatsDirtyRate(virQEMUDriver *driver,
>                              virDomainObj *dom,
> @@ -18539,8 +18541,9 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver,
>      if (qemuDomainGetStatsDirtyRateMon(driver, dom, &info) < 0)
>          return -1;
>  
> -    if (virTypedParamListAddInt(params, info.status,
> -                                "dirtyrate.calc_status") < 0)
> +    if (virTypedParamListAddString(params,
> +                                   qemuMonitorDirtyRateStatusTypeToString(info.status),
> +                                   "dirtyrate.calc_status") < 0)

I worry that this change is not backwards compatible. I mean, what if I
had an app that fetches dirtyrate.calc_status, expecting it to be an int?

In some languages this may be less of a problem (e.g. in python) but
still, I would have to make changes to my app only because of this patch.

Michal