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@xxxxxxxxxx>
>> ---
>> 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
>
> .
>
