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

On 2/16/22 01:28, huangy81(a)chinatelecom.cn wrote: > From: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn&gt; > > 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(a)chinatelecom.cn&gt; > --- > 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.