Re: [PATCH 03/15] util: cgroup: Use GHashTable instead of virHashTable

[Pavel Hrdina]

On Thu, Oct 22, 2020 at 11:34:54AM +0200, Peter Krempa wrote:
> Rewrite using GHashTable which already has interfaces for using a number
> as hash key. Glib's implementation doesn't copy the key by default, so
> we need to allocate it, but overal the interface is more suited for this
> case.
> 
> Signed-off-by: Peter Krempa <pkre...@redhat.com>
> ---
>  src/util/vircgroup.c        | 61 ++++++++-----------------------------
>  src/util/vircgroupbackend.h |  3 +-
>  src/util/vircgrouppriv.h    |  2 +-
>  src/util/vircgroupv1.c      |  2 +-
>  src/util/vircgroupv2.c      |  2 +-
>  5 files changed, 17 insertions(+), 53 deletions(-)
> 
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 5f4cb01bc0..b74ec1a8fa 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -42,7 +42,6 @@
>  #include "virlog.h"
>  #include "virfile.h"
>  #include "virhash.h"
> -#include "virhashcode.h"
>  #include "virstring.h"
>  #include "virsystemd.h"
>  #include "virtypedparam.h"
> @@ -2382,7 +2381,7 @@ virCgroupRemove(virCgroupPtr group)
>  static int
>  virCgroupKillInternal(virCgroupPtr group,
>                        int signum,
> -                      virHashTablePtr pids,
> +                      GHashTable *pids,
>                        int controller,
>                        const char *taskFile)
>  {
> @@ -2415,8 +2414,9 @@ virCgroupKillInternal(virCgroupPtr group,
>              goto cleanup;
>          } else {
>              while (!feof(fp)) {
> -                long pid_value;
> -                if (fscanf(fp, "%ld", &pid_value) != 1) {
> +                g_autofree long long *pid_value = g_new0(long long, 1);

I would rather use gint64 here as the exact type of gint64 changes with
the hardware. For example on my AMD x86_84 it is 'signed long'.

We should do this every time we pass pointers to GLib APIs because for
example bool and gboolean are different and when I used bool type in
GLib dbus APIs it randomly crashed.

> +                if (fscanf(fp, "%lld", pid_value) != 1) {
>                      if (feof(fp))
>                          break;
>                      virReportSystemError(errno,
> @@ -2424,16 +2424,17 @@ virCgroupKillInternal(virCgroupPtr group,
>                                           keypath);
>                      goto cleanup;
>                  }
> -                if (virHashLookup(pids, (void*)pid_value))
> +
> +                if (g_hash_table_lookup(pids, pid_value))
>                      continue;
> 
> -                VIR_DEBUG("pid=%ld", pid_value);
> +                VIR_DEBUG("pid=%lld", *pid_value);

Using gint64 would require to use G_GINT64_FORMAT here.

Otherwise looks good.

Pavel

signature.asc
Description: PGP signature