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