[igt-dev] [PATCH i-g-t v7 1/3] Introduce device selection API

[Zbigniew Kempczyński]

On Thu, Sep 12, 2019 at 01:59:46PM +0300, Arkadiusz Hiler wrote:

<cut>

> > +/* Scanned devices */
> > +struct igt_devices {
> > +	GPtrArray *devs;		//all devices
> > +	GPtrArray *view;		//filtered view
> 
> use /* */
> 
> or even better, name them "all" and "filtered"
>

Before I've commented this using // I checked already 
existing sources. Such comments exists, even they were added this 
year. 

> > +	g_ptr_array_sort(igt_devs.devs, devs_compare);
> > +	for (int i = 0; i < igt_devs.devs->len; i++) {
> > +		g_ptr_array_add(igt_devs.view,
> > +				g_ptr_array_index(igt_devs.devs, i));
> > +	}
> > +}
> 
> The only part of the whole codebase that seems to really benefit from
> glib are the hashtables for device pops and attrs. Everything else can
> just easily be written in plain C + [igt_list].
> 
> The amount of glib code is the main reason why it is so hard to get
> anyone to review this. Most people working on IGT/kernel are not used to
> work with it and it increases cognitive strain significantly. I have
> bounced off myself few times on the more intense glib action.
> 
> I believe you have been warned about this ahead of time.
> 
> [igt_list]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/lib/igt_list.h

This is my personal preference during coding this part. I like glib
arrays - they allow auto resize and keep better cache locality 
than lists. Mixing glib and igt structure would looks strange IMO.
I wondered about pure glibc data structures but hash tables 
in glibc doesn't handle conflicts so I reject them. 

I see no reason why should I change this implementation to other
data structure especially I've test this code with valgrind to
avoid memory leaks especially if we will need call device
scanning function again.

<cut>

> > +
> > +struct filter_func {
> 
> this is a bit more than a filter function, filter_definiton?
> 
> > +	GPtrArray *(*filter_function)(struct filter_spec *fspec,
> > +				      struct filter_func *ffunc);
> > +	const char *help;
> > +	const char *detail;
> > +
> > +	const char *property;
> > +	const char *value;
> > +};

Ok this seems better than filter_func. I'll push it to v8.

<cut>

> > +static GPtrArray *filter_sys(struct filter_spec *fspec,
> > +			     struct filter_func *ffunc)
> > +{
> > +	(void) ffunc;
> > +
> > +	DBG("filter sys\n");
> > +	if (!strlen(fspec->data))
> > +		return igt_devs.view;
> > +
> > +	for (int i = 0; i < igt_devs.devs->len; i++) {
> > +		struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i);
> > +
> > +		if (strequal(dev->subsystem, fspec->name) &&
> > +			strequal(dev->syspath, fspec->data)) {
> > +			g_ptr_array_add(igt_devs.view, dev);
> > +			break;
> > +		}
> > +	}
> 
> this looks suspiciously similar to igt_device_find and probably can be
> based on it
> 
> > +
> > +	DBG("Filter sys view size: %d\n", igt_devs.view->len);
> > +
> > +	return igt_devs.view;
> > +}

You're right. I'll reuse this function.

<cut> 

> > +
> > +	/* Clean view */
> > +	g_ptr_array_remove_range(igt_devs.view, 0, igt_devs.view->len);
> > +
> > +	/* If fspec.data contains "/sys" use direct path instead
> > +	 * contextual filter.
> > +	 */
> > +	if (!strncmp(fspec.data, "/sys", 4))
> > +		filter_sys(&fspec, NULL);
> 
> I don't like that we have special case for *:/sys here and in
> is_filter_valid().
> 
> How about instead of using hash table for matching fspec.name doing
> something like that:
> 
> extend filter_definition aka (filter_func) with:
>     bool (*is_the_right_filter)(struct filter_spec *fspec);
>     char *name;
>     bool (*is_valid)(struct filter_spec *fspec, struct filter_definition *fdef);
> 
> Instead of hashtable you can keep them in array (order matters), and:
> 
> is_the_right_filter for sys would be:
> 	return !strncmp(fspec.data, "/sys", 4))
> 
> and the a single function that matches name for the others
> 
> filter_definition *get_filter_for_spec(*fspec) {
> 	for (def in filter_definitions) {
> 		if (def->is_the_right_filter && def->is_the_right_filter(fspec))
> 			return def;
> 	}
> 
> 	return NULL;
> }
> 
> And then:
> 
> bool is_filter_valid(*fspec) {
> 	struct filter_defintion def = NULL;
> 	def = get_filter_for_spec(fspec);
> 
> 	if (def != NULL && def->is_valid != NULL)
> 		return def->is_valid(fspec);
> 
> 	retrun false;
> }
> 
> this limits the need for glib magic (ht, gpointers) and makes the magic
> checks more localized to the filter definition.

We're doing bisect in modprobe stuff with few elements so I've choosen
hash tables and ptr array do keep internal implementation fast. Apart
of single exception with '/sys' where I assume user want to select 
exact card. So I can't guess subsystem in subsystem:/sys/... path
and make appropriate hash table entry.

This makes confusion on me. IGT uses glib for parsing .igtrc but
developers resists to any glib data structures.

> 
> > +	else {
> > +		struct filter_func *ffunc;
> > +
> > +		ffunc = g_hash_table_lookup(filter_definition_ht, fspec.name);
> > +		if (!ffunc) {
> > +			igt_warn("No filter with name [%s]\n", fspec.name);
> > +			return false;
> > +		}
> > +		ffunc->filter_function(&fspec, ffunc);
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +#define safe_strncpy(dst, src, size) do { \
> > +	if ((dst) && (src)) \
> > +		strncpy((dst), (src), (size)); \
> > +	} while (0)
> 
> I would go with if (src != NULL) for the three uses down there (we
> already make sure that (card != NULL), so dst is ok).

I think both arguments should be checked before strcpy.

> 
> 
> > +/**
> > + * igt_device_card_match
> > + * @filter: filter string
> > + * @card: pointer to igt_device_card struct
> > + *
> > + * Function applies filter to match device from device array.
> > + *
> > + * Returns:
> > + * false - no card pointer was passed or card wasn't matched,
> > + * true - card matched and returned.
> > + */
> > +bool igt_device_card_match(const char *filter, struct igt_device_card *card)
> > +{
> > +	struct igt_device *dev = NULL;
> > +
> > +	if (!card)
> > +		return false;
> > +	memset(card, 0, sizeof(*card));
> > +
> > +	igt_devices_scan(false);
> > +
> > +	if (igt_device_filter_apply(filter) == false)
> > +		return false;
> > +
> > +	if (!igt_devs.view->len)
> > +		return false;
> > +
> > +	/* We take first one if more than one card matches filter */
> > +	dev = g_ptr_array_index(igt_devs.view, 0);
> > +	card->chipset = DRIVER_ANY;
> > +	if (dev->vs)
> > +		card->chipset = dev->vs->chipset;
> > +
> > +	safe_strncpy(card->subsystem, dev->subsystem, NAME_MAX);
> > +	safe_strncpy(card->card, dev->drm_card, NAME_MAX);
> > +	safe_strncpy(card->render, dev->drm_render, NAME_MAX);
> 
> A better option would be to use sizeof(card->subsystem), etc.

Agree.

Zbigniew