[PATCH v2 2/3] util: Add virGetCpuHaltPollTime and virGetDebugFsKvmValue

[Yang Fei]

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 at 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
> 
> .
>