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

[Hyman Huang <huangy81@xxxxxxxxxxxxxxx>]

在 2022/2/18 0:19, Michal Prívozník 写道:
> On 2/16/22 01:28, huangy81@xxxxxxxxxxxxxxx wrote:
> > From: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> >   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.
Indeed, there exists such scenario, so two policy can be choosed:
1. just drop the commit and ignore
2. add a new field like "dirtyrate.calc_ret_status" to reprensent these.

which do you prefer?

> Michal

--
Best regard

Hyman Huang(黄勇)