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

[Daniel P. Berrangé]

On Thu, Feb 17, 2022 at 05:19:32PM +0100, Michal Prívozník wrote:
> 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.

Yes, that'll certainly break the Go bindings as we expose everything
in stronglky typed structs.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|