On 7/16/21 12:42 PM, Yang Fei wrote:
> Add helper function virGetCpuHaltPollTime to obtain halt polling
> time. If the kernel support halt polling time statistic, and mount
> debugfs. This function will take effect on KVM VMs.
>
> Signed-off-by: Yang Fei <yangfei85@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 43 ++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 4 ++++
> 3 files changed, 48 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 68e4b6aab8..64aff4eca4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3479,6 +3479,7 @@ virDoesUserExist;
> virDoubleToStr;
> virFormatIntDecimal;
> virFormatIntPretty;
> +virGetCpuHaltPollTime;
> virGetDeviceID;
> virGetDeviceUnprivSGIO;
> virGetGroupID;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 311cbbf93a..f5304644c0 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
> {
> return virPipeImpl(fds, true, true);
> }
> +
Apart from Peter's findings:
I'm not sure virutil.c is the best placement for this function. I mean,
virutil.c became dump of functions we did not find a better place for.
How about src/util/virhostcpu.c ? That seems like a better fit, doesn't it?
> +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;
> + g_autoptr(DIR) dir = NULL;
> + int ret = -1;
This variable is pretty much useless. The reason we have it in other
functions is that they have the following pattern:
int ret = -1;
if ()
goto clenaup;
if ()
goto cleanup;
ret = 0;
cleanup:
something();
return ret;
But this is not really the case for this function. You can do 'return
-1' instead of 'return ret' and then plain 'return 0' at the end.
> + bool found = false;
> +
> + if (!(debugFsPath = virFileFindMountPoint("debugfs")))
> + return ret;
> +
> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
> + if (virDirOpenIfExists(&dir, completePath) != 1)
> + return ret;
> +
> + pidToStr = g_strdup_printf("%d%c", pid, '-');
> + while (virDirRead(dir, &ent, NULL) > 0) {
> + if (STRPREFIX(ent->d_name, pidToStr)) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return ret;
> +
> + if (virFileReadValueUllong(haltPollSuccess, "%s/%s/%s", completePath,
> + ent->d_name, "halt_poll_success_ns") < 0
> + || virFileReadValueUllong(haltPollFail, "%s/%s/%s", completePath,
> + ent->d_name, "halt_poll_fail_ns") < 0) {
We like to format this as:
if (condition1 ||
condition2)
> + return ret;
> + }
> +
> + ret = 0;
> +
> + return ret;
> +}
Michal
