[PATCH v3 1/2] util: Add virGetCpuHaltPollTime

[Michal Prívozník]

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