On 2021/7/14 22:17, Michal Prívozník wrote:
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 <yangfei85(a)huawei.com>
> ---
> 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.
I will directly use the function virFileReadValueUllong() in the next version.
> +
> +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?
If kernel doesn't have debugfs, halt polling time will not be presented in domstats,
and return what it can obtain.
Could I use VIR_WARN() or VIR_INFO() to give the fail messege?
> +
> + 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?
Yes, virDirOpenIfExists() is indeed more appropriate. I will fix it next version.
> +
> + 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 domain runs TCG, the pid-fd directory will not be generated in the path. Domstats just
return the
data it can obtain. That's really not appropriate to report a error here.
And I'll give the variable a more appropriate name in next version.
> +
> + 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;
I'll pay more attention about it and modify the definition method in next version.
> + 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
.