[PATCH i-g-t v10 1/5] lib/igt_device_scan: Add support for the device filter

[Krzysztof Karas]

Hi Soham,

[...]

> +	if (filter->data.card) {
> +		char crdop[5] = {0};
> +
> +		if (sscanf(filter->data.card, "%d", &card) == 1) {
> +			if (card < 0)
> +				return &igt_devs.filtered;
> +		} else {
> +			card = 0;
> +			if (sscanf(filter->data.card, "%4s", crdop) == 1) {
> +				if (!strcmp(crdop, "all"))
> +					allcards = true;
> +				else
> +					return &igt_devs.filtered;
> +			} else {
> +				return &igt_devs.filtered;
> +			}
> +		}
> +	} else {
> +		card = 0;
> +	}

I rearranged the code a bit to make the i look a bit more
concise:

if (filter->data.card) {
        char crdop[5] = {0};

	if (sscanf(filter->data.card, "%d", &card) != 1) {
		card = 0;
                if (sscanf(filter->data.card, "%4s", crdop) != 1)
			return &igt_devs.filtered;

		if (strcmp(crdop, "all") != 0)
			return &igt_devs.filtered;

		allcards = true;
        }
}

if (card < 0)
        return &igt_devs.filtered;

I believe it is logically equivalent. I ran a bit of testing via
lsgpu tool and did not notice any changes from your version.
I thought that we could reduce the number of "else" blocks and
decrease indentation a little bit. Let me know what you think
and if you could include this part in your patch (or if you find
that this suggestion is bogus and does not make sense ;) ).

> +
> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> +		/* Skip if 'driver' doesn't match */
> +		if (filter->data.driver && !strequal(filter->data.driver, dev->driver))
> +			continue;
> +
> +		/* Skip if 'device' doesn't match */
> +		if (filter->data.device && !is_device_matched(dev, filter->data.device))
> +			continue;
> +
> +		if (filter->data.subsystem && strcmp(filter->data.subsystem, "all")) {

I see that you added a neat comment to most checks around here,
except this subsystem filter. Do you think you could add
"/* Skip if 'subsystem' doesn't match */" comment here?
Or, if you deem that suggestion excessive, please drop the
comments from the checks above - these ifs are quite
self-explanatory.
> +			if (strcmp(filter->data.subsystem, get_prop_subsystem(dev)))
> +				continue;
> +		}
> +

Best Regards,
Krzysztof