Great thanks for these helpful suggestions. I'll introduce them into my next version.
On 2020/11/11 4:11, Michal Privoznik wrote:
> On 11/7/20 10:41 AM, Hao Wang wrote:
>> Introduce DomainGetDirtyRateInfo API for domain's memory dirty rate
>> calculation and query.
>>
>> Signed-off-by: Hao Wang <wanghao232@xxxxxxxxxx>
>> Signed-off-by: Zhou Yimin <zhouyimin@xxxxxxxxxx>
>> Reviewed-by: Chuan Zheng <zhengchuan@xxxxxxxxxx>
>> ---
>> include/libvirt/libvirt-domain.h | 5 ++++
>> src/driver-hypervisor.h | 7 +++++
>> src/libvirt-domain.c | 46 ++++++++++++++++++++++++++++++++
>> src/libvirt_public.syms | 5 ++++
>> src/remote/remote_driver.c | 1 +
>> src/remote/remote_protocol.x | 21 ++++++++++++++-
>> 6 files changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
>> index 145f517068..b950736b67 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -5120,4 +5120,9 @@ struct _virDomainDirtyRateInfo {
>> typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr;
>> +int virDomainGetDirtyRateInfo(virDomainPtr domain,
>> + virDomainDirtyRateInfoPtr info,
>> + long long sec,
>> + unsigned int flags);
>> +
>> #endif /* LIBVIRT_DOMAIN_H */
>> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
>> index bce023017d..a77c29de54 100644
>> --- a/src/driver-hypervisor.h
>> +++ b/src/driver-hypervisor.h
>> @@ -1387,6 +1387,12 @@ typedef char *
>> (*virDrvDomainBackupGetXMLDesc)(virDomainPtr domain,
>> unsigned int flags);
>> +typedef int
>> +(*virDrvDomainGetDirtyRateInfo)(virDomainPtr domain,
>> + virDomainDirtyRateInfoPtr info,
>> + long long sec,
>> + unsigned int flags);
>> +
>> typedef struct _virHypervisorDriver virHypervisorDriver;
>> typedef virHypervisorDriver *virHypervisorDriverPtr;
>> @@ -1650,4 +1656,5 @@ struct _virHypervisorDriver {
>> virDrvDomainAgentSetResponseTimeout domainAgentSetResponseTimeout;
>> virDrvDomainBackupBegin domainBackupBegin;
>> virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc;
>> + virDrvDomainGetDirtyRateInfo domainGetDirtyRateInfo;
>> };
>> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
>> index 3c5f55176a..ce3c40edf8 100644
>> --- a/src/libvirt-domain.c
>> +++ b/src/libvirt-domain.c
>> @@ -12758,3 +12758,49 @@ virDomainBackupGetXMLDesc(virDomainPtr domain,
>> virDispatchError(conn);
>> return NULL;
>> }
>> +
>> +
>> +/**
>> + * virDomainGetDirtyRateInfo:
>> + * @domain: a domain object.
>> + * @info: return value of current domain's memory dirty rate info.
>> + * @sec: show dirty rate within specified seconds.
>> + * @flags: the flags of getdirtyrate action -- calculate and/or query.
>
> What are the flags? Which enum should I look at?
>
>> + *
>> + * Get the current domain's memory dirty rate (in MB/s).
>
> Can you expand on this a bit? Look at description to other APIs for inspiration.
>
>> + *
>> + * Returns 0 in case of success, -1 otherwise.
>> + */
>> +int
>> +virDomainGetDirtyRateInfo(virDomainPtr domain,
>> + virDomainDirtyRateInfoPtr info,
>> + long long sec,
>
> Do we really need long long? That's more than 2.9*10^11 years. I don't think any virtual machine will ever run for so long (considering that the age of the universe is ~1.38*10^10 years)
>
>> + unsigned int flags)
>> +{
>> + virConnectPtr conn;
>> +
>> + VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec);
>
> @flags should also be logged.
>
>> +
>> + virResetLastError();
>> +
>> + virCheckDomainReturn(domain, -1);
>> + conn = domain->conn;
>> +
>> + virCheckNonNullArgGoto(info, error);
>> + virCheckReadOnlyGoto(conn->flags, error);
>> +
>> + if (info)
>> + memset(info, 0, sizeof(*info));
>
> @info was checked for NULL just two lines above.
>
>> +
>> + if (conn->driver->domainGetDirtyRateInfo) {
>> + if (conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags) < 0)
>> + goto error;
>> + VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec);
>
> This is not very useful debug log.
>
>> + return 0;
>> + }
>> +
>> + virReportUnsupportedError();
>> + error:
>> + virDispatchError(conn);
>> + return -1;
>> +}
>> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
>> index 539d2e3943..11864f48b1 100644
>> --- a/src/libvirt_public.syms
>> +++ b/src/libvirt_public.syms
>> @@ -873,4 +873,9 @@ LIBVIRT_6.0.0 {
>> virDomainBackupGetXMLDesc;
>> } LIBVIRT_5.10.0;
>> +LIBVIRT_6.9.0 {
>> + global:
>> + virDomainGetDirtyRateInfo;
>> +} LIBVIRT_6.0.0;
>> +
>
> Unfortunately, this missed 6.9.0 so this must be 6.10.0 instead.
>
>> # .... define new API here using predicted next version number ....
>> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
>> index 9cd2fd36ae..325968a22b 100644
>> --- a/src/remote/remote_driver.c
>> +++ b/src/remote/remote_driver.c
>> @@ -8458,6 +8458,7 @@ static virHypervisorDriver hypervisor_driver = {
>> .domainAgentSetResponseTimeout = remoteDomainAgentSetResponseTimeout, /* 5.10.0 */
>> .domainBackupBegin = remoteDomainBackupBegin, /* 6.0.0 */
>> .domainBackupGetXMLDesc = remoteDomainBackupGetXMLDesc, /* 6.0.0 */
>> + .domainGetDirtyRateInfo = remoteDomainGetDirtyRateInfo, /* 6.9.0 */
>
> Same here.
>
>> };
>> static virNetworkDriver network_driver = {
>
>
> I think the following should be squashed in:
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index e8bd890b29..594d7a2790 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -5096,28 +5096,18 @@ int virDomainBackupBegin(virDomainPtr domain,
> char *virDomainBackupGetXMLDesc(virDomainPtr domain,
> unsigned int flags);
>
> -/**
> - * virDomainDirtyRateFlags:
> - *
> - * Details on the flags used by getdirtyrate api.
> - */
> -
> -typedef enum {
> - VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirtyrate */
> - VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */
> -} virDomainDirtyRateFlags;
> -
> /**
> * virDomainDirtyRateStatus:
> *
> - * Details on the cause of a dirtyrate calculation status.
> + * 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_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 */
> + completed */
>
> # ifdef VIR_ENUM_SENTINELS
> VIR_DOMAIN_DIRTYRATE_LAST
> @@ -5133,12 +5123,23 @@ completed */
> typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo;
> typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr;
> struct _virDomainDirtyRateInfo {
> - int status; /* the status of dirty rate calculation */
> + int status; /* the status of dirty rate calculation, one of
> + virDomainDirtyRateStatus */
> long long dirtyRate; /* the dirty rate in MB/s */
> long long startTime; /* the start time of dirty rate calculation */
> long long calcTime; /* the period of dirty rate calculation */
> };
>
> +/**
> + * virDomainDirtyRateFlags:
> + *
> + * Details on the flags used by getdirtyrate api.
> + */
> +typedef enum {
> + VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirty rate */
> + VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirty rate */
> +} virDomainGetDirtyRateInfoFlags;
> +
> int virDomainGetDirtyRateInfo(virDomainPtr domain,
> virDomainDirtyRateInfoPtr info,
> long long sec,
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index ce3c40edf8..75029b0a4b 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -12762,10 +12762,10 @@ virDomainBackupGetXMLDesc(virDomainPtr domain,
>
> /**
> * virDomainGetDirtyRateInfo:
> - * @domain: a domain object.
> - * @info: return value of current domain's memory dirty rate info.
> - * @sec: show dirty rate within specified seconds.
> - * @flags: the flags of getdirtyrate action -- calculate and/or query.
> + * @domain: a domain object
> + * @info: pointer to current domain's memory dirty rate info
> + * @sec: show dirty rate within specified seconds
> + * @flags: extra flags; binary-OR of virDomainGetDirtyRateInfoFlags
> *
> * Get the current domain's memory dirty rate (in MB/s).
> *
> @@ -12779,7 +12779,8 @@ virDomainGetDirtyRateInfo(virDomainPtr domain,
> {
> virConnectPtr conn;
>
> - VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec);
> + VIR_DOMAIN_DEBUG(domain, "info=%p, sec=%lld, flags=0x%x",
> + info, sec, flags);
>
> virResetLastError();
>
> @@ -12789,14 +12790,14 @@ virDomainGetDirtyRateInfo(virDomainPtr domain,
> virCheckNonNullArgGoto(info, error);
> virCheckReadOnlyGoto(conn->flags, error);
>
> - if (info)
> - memset(info, 0, sizeof(*info));
> + memset(info, 0, sizeof(*info));
>
> if (conn->driver->domainGetDirtyRateInfo) {
> - if (conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags) < 0)
> + int ret;
> + ret = conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags);
> + if (ret < 0)
> goto error;
> - VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec);
> - return 0;
> + return ret;
> }
>
> virReportUnsupportedError();
>
> Michal
>
> .
