On Fri, Jul 16, 2021 at 18:42:22 +0800, 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);
> }
> +
> +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;
> + bool found = false;
> +
> + if (!(debugFsPath = virFileFindMountPoint("debugfs")))
> + return ret;
Here '-1' is returned, but no libvirt error is reported ...
> +
> + completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
> + if (virDirOpenIfExists(&dir, completePath) != 1)
> + return ret;
While here -1 is reported and an error may be reported (e.g on EACCESS).
We generally avoid this pattern because the caller cant properly handle
that.
> +
> + pidToStr = g_strdup_printf("%d%c", pid, '-');
I'm fairly certain this will fail to compile on mingw since %d isn't the
proper formatting modifier for pid_t.
> + while (virDirRead(dir, &ent, NULL) > 0) {
> + if (STRPREFIX(ent->d_name, pidToStr)) {
Any idea what the suffix after '-' in the debugfs entry is? If we can
figure it out we won't have to look up the correct file.
> + 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) {
As you can see here, this is one of the functions having terrible
semantics. It sometimes does set the libvirt error object and sometimes
doesn't, thats why we try to avoid those.
> + return ret;
> + }
> +
> + ret = 0;
> +
> + return ret;
> +}
