[PATCH v4 15/15] lib/igt_device_scan: reuse igt hash

[Zbigniew Kempczyński]

On Wed, May 07, 2025 at 04:01:48PM +0000, Jeevaka Prabu Badrappan wrote:
> Signed-off-by: Jeevaka Prabu Badrappan <jeevaka.badrappan at intel.com>
> ---
>  lib/igt_device_scan.c | 76 ++++++++++++++++++++++---------------------
>  lib/meson.build       |  1 +
>  2 files changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index e23943a10..7d54886dd 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -26,12 +26,12 @@
>  #include "igt_core.h"
>  #include "igt_device_scan.h"
>  #include "igt_list.h"
> +#include "igt_map.h"
>  #include "intel_chipset.h"
>  
>  #include <ctype.h>
>  #include <dirent.h>
>  #include <fcntl.h>
> -#include <glib.h>

I've nothing against replacing glib hashmap to igt_map, but there're
two issues here:

>  #include <libudev.h>
>  #ifdef __linux__
>  #include <linux/limits.h>
> @@ -224,8 +224,8 @@ struct igt_device {
>  	/* Point to vendor spec if can be found */
>  
>  	/* Properties / sysattrs rewriten from udev lists */
> -	GHashTable *props_ht;
> -	GHashTable *attrs_ht;
> +	struct igt_map *props_map;
> +	struct igt_map *attrs_map;
>  
>  	/* Most usable variables from udev device */
>  	char *subsystem;
> @@ -464,6 +464,11 @@ static bool is_on_blacklist(const char *what)
>  
>  }
>  
> +static int key_equals(const void *key1, const void *key2)
> +{
> +	return strcmp((char *)key1, (char *)key2) == 0;
> +}
> +
>  static struct igt_device *igt_device_new(void)
>  {
>  	struct igt_device *dev;
> @@ -472,14 +477,14 @@ static struct igt_device *igt_device_new(void)
>  	if (!dev)
>  		return NULL;
>  
> -	dev->attrs_ht = g_hash_table_new_full(g_str_hash, g_str_equal,
> -					      free, free);
> -	dev->props_ht = g_hash_table_new_full(g_str_hash, g_str_equal,
> -					      free, free);
> +	dev->attrs_map = igt_map_create(igt_map_hash_32, key_equals);
> +	dev->props_map = igt_map_create(igt_map_hash_32, key_equals);
>  
> -	if (dev->attrs_ht && dev->props_ht)
> +	if (dev->attrs_map && dev->props_map)
>  		return dev;
>  
> +	igt_map_destroy(dev->attrs_map, NULL);
> +	igt_map_destroy(dev->props_map, NULL);

What for are you freeing the map here?

>  	free(dev);
>  
>  	return NULL;
> @@ -491,7 +496,7 @@ static void igt_device_add_prop(struct igt_device *dev,
>  	if (!key || !value)
>  		return;
>  
> -	g_hash_table_insert(dev->props_ht, strdup(key), strdup(value));
> +	igt_map_insert(dev->props_map, strdup(key), strdup(value));
>  }
>  
>  static void igt_device_add_attr(struct igt_device *dev,
> @@ -525,7 +530,7 @@ static void igt_device_add_attr(struct igt_device *dev,
>  		v++;
>  	}
>  
> -	g_hash_table_insert(dev->attrs_ht, strdup(key), strdup(v));
> +	igt_map_insert(dev->attrs_map, strdup(key), strdup(value));

This is invalid and lead to segv. Use v instead of value.

Apart of this I haven't found more issues in the patch.

--
Zbigniew

>  }
>  
>  /* Iterate over udev properties list and rewrite it to igt_device properties
> @@ -585,19 +590,30 @@ static void get_attrs_limited(struct udev_device *dev, struct igt_device *idev)
>  	}
>  }
>  
> -#define get_prop(dev, prop) ((char *) g_hash_table_lookup(dev->props_ht, prop))
> -#define get_attr(dev, attr) ((char *) g_hash_table_lookup(dev->attrs_ht, attr))
> +#define get_prop(dev, prop) ((char *) igt_map_search(dev->props_map, prop))
> +#define get_attr(dev, attr) ((char *) igt_map_search(dev->attrs_map, attr))
>  #define get_prop_subsystem(dev) get_prop(dev, "SUBSYSTEM")
>  #define is_drm_subsystem(dev)  (strequal(get_prop_subsystem(dev), "drm"))
>  #define is_pci_subsystem(dev)  (strequal(get_prop_subsystem(dev), "pci"))
>  
> -static void print_ht(GHashTable *ht);
> +static inline void _print_key_value(const char *k, const char *v)
> +{
> +	printf("%-32s: %s\n", k, v);
> +}
> +
>  static void dump_props_and_attrs(const struct igt_device *dev)
>  {
> +	struct igt_map_entry *entry;
> +
>  	printf("\n[properties]\n");
> -	print_ht(dev->props_ht);
> +	igt_map_foreach(dev->props_map, entry) {
> +		_print_key_value((char *)entry->key, (char *)entry->data);
> +	}
> +
>  	printf("\n[attributes]\n");
> -	print_ht(dev->attrs_ht);
> +	igt_map_foreach(dev->attrs_map, entry) {
> +		_print_key_value((char *)entry->key, (char *)entry->data);
> +	}
>  	printf("\n");
>  }
>  
> @@ -1076,6 +1092,12 @@ static void scan_drm_devices(bool limit_attrs)
>  	}
>  }
>  
> +static void free_key_value(struct igt_map_entry *entry)
> +{
> +	free((char *)entry->key);
> +	free(entry->data);
> +}
> +
>  static void igt_device_free(struct igt_device *dev)
>  {
>  	free(dev->codename);
> @@ -1088,8 +1110,8 @@ static void igt_device_free(struct igt_device *dev)
>  	free(dev->device);
>  	free(dev->driver);
>  	free(dev->pci_slot_name);
> -	g_hash_table_destroy(dev->attrs_ht);
> -	g_hash_table_destroy(dev->props_ht);
> +	igt_map_destroy(dev->attrs_map, free_key_value);
> +	igt_map_destroy(dev->props_map, free_key_value);
>  }
>  
>  void igt_devices_free(void)
> @@ -1323,26 +1345,6 @@ igt_devs_print_user(struct igt_list_head *view,
>  	}
>  }
>  
> -static inline void _print_key_value(const char* k, const char *v)
> -{
> -	printf("%-32s: %s\n", k, v);
> -}
> -
> -static void print_ht(GHashTable *ht)
> -{
> -	GList *keys = g_hash_table_get_keys(ht);
> -
> -	keys = g_list_sort(keys, (GCompareFunc) strcmp);
> -	while (keys) {
> -		char *k = (char *) keys->data;
> -		char *v = g_hash_table_lookup(ht, k);
> -
> -		_print_key_value(k, v);
> -		keys = g_list_next(keys);
> -	}
> -	g_list_free(keys);
> -}
> -
>  static void
>  igt_devs_print_detail(struct igt_list_head *view,
>  		      const struct igt_devices_print_format *fmt)
> diff --git a/lib/meson.build b/lib/meson.build
> index b58976a43..34979b42c 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -307,6 +307,7 @@ lib_igt_device_scan_build = static_library('igt_device_scan',
>  	'igt_tools_stub.c',
>  	'intel_device_info.c',
>  	'intel_cmds_info.c',
> +	'igt_map.c',
>  	],
>  	dependencies : scan_dep,
>  	include_directories : inc)
> -- 
> 2.34.1
>