On 7/14/21 2:28 PM, Yang Fei wrote:
> Add helper function virGetCpuHaltPollTime and virGetDebugFsKvmValue
> to obtain the halt polling time. If system mount debugfs, and the
> kernel support halt polling time statistic, it will work.
>
> Signed-off-by: Yang Fei <[email protected]>
> ---
> src/libvirt_private.syms | 2 ++
> src/util/virutil.c | 78 ++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 9 +++++
> 3 files changed, 89 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 68e4b6aab8..f92213b8c2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3479,6 +3479,8 @@ virDoesUserExist;
> virDoubleToStr;
> virFormatIntDecimal;
> virFormatIntPretty;
> +virGetCpuHaltPollTime;
> +virGetDebugFsKvmValue;
> virGetDeviceID;
> virGetDeviceUnprivSGIO;
> virGetGroupID;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index c0d25fe247..7e4531b4b4 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1959,3 +1959,81 @@ virGetValueRaw(const char *path,
>
> return 0;
> }
> +
> +int
> +virGetDebugFsKvmValue(struct dirent *ent,
> + const char *path,
> + const char *filename,
> + unsigned long long *value)
> +{
> + g_autofree char *valToStr = NULL;
> + g_autofree char *valPath = NULL;
> +
> + valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename);
> +
> + if (virGetValueRaw(valPath, &valToStr) < 0) {
> + return -1;
> + }
> +
> + /* 10 is a Cardinality, must be between 2 and 36 inclusive,
> + * or special value 0. Used in fuction strtoull()
> + */
> + if (virStrToLong_ull(valToStr, NULL, 10, value) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to parse '%s' as an integer"), valToStr);
> + return -1;
> + }
> +
> + return 0;
> +}
This looks very close to what virFileReadValueUllong() does.
> +
> +int
> +virGetCpuHaltPollTime(pid_t pid,
> + unsigned long long *haltPollSuccess,
> + unsigned long long *haltPollFail)
> +{
> + g_autofree char *pidToStr = NULL;
> + g_autofree char *debugFsPath = NULL;
> + g_autofree char *completePath = NULL;
> + struct dirent *ent = NULL;
> + DIR *dir = NULL;
> + int ret = -1;
> + int flag = 0;
> +
> + if (!(debugFsPath = virFileFindMountPoint("debugfs"))) {
> + virReportSystemError(errno, "%s",
> + _("unable to find debugfs mountpoint"));
> + return ret;
> + }
Is this something worth polluting logs with error? What if we run under
kernel that doesn't have debugfs?
> +
> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
> + if (virDirOpen(&dir, completePath) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s %s", "Can not open directory", completePath);
> + return ret;
> + }
virDirOpen() reports an error on failure. No need to rewrite it. And in
the light of previous comment maybe virDirOpenIfExists() instead?
> +
> + pidToStr = g_strdup_printf("%lld%c", (unsigned long long)pid, '-');
> + while (virDirRead(dir, &ent, NULL) > 0) {
> + if (STRPREFIX(ent->d_name, pidToStr)) {
> + flag = 1;
> + break;
> + }
> + }
> +
> + if (flag == 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not find VM(Pid %s) in '%s'"), pidToStr,
> completePath);
> + goto cleanup;
> + }
Again, not big fan. What it my domain runs TCG? Also, please rename
'flag' to something more specific, e.g. "found".
> +
> + if (virGetDebugFsKvmValue(ent, completePath, "halt_poll_success_ns",
> haltPollSuccess) < 0 ||
> + virGetDebugFsKvmValue(ent, completePath, "halt_poll_fail_ns",
> haltPollFail) < 0) {
> + goto cleanup;
> + }
> +
> + ret = 0;
> +cleanup:
> + closedir(dir);
Again, please make sure 'ninja test' passes. We have a syntax-check rule
that forbids direct call of closedir(). The proper way is to define dir
variable like this:
g_autoptr(DIR) dir = NULL;
> + return ret;
> +}
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 851b421476..00cef47208 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -228,3 +228,12 @@ int virPipeNonBlock(int fds[2]);
>
> int virGetValueRaw(const char *path,
> char **value);
> +
> +int virGetDebugFsKvmValue(struct dirent *ent,
> + const char *path,
> + const char *filename,
> + unsigned long long *value);
> +
> +int virGetCpuHaltPollTime(pid_t pid,
> + unsigned long long *haltPollSuccess,
> + unsigned long long *haltPollFail);
>
Michal
