Re: [PATCH i-g-t 2/2] lib/igt_drm_fdinfo: Mandatory drm-fields first

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 01/02/2024 00:46, Lucas De Marchi wrote:
> drm-driver and drm-client-id are 2 keys that should be on any driver's
> implementation of fdinfo. Move them earlier so it's clear what the
> "good" tracking is doing.

This change is a bit meh since I could argue the order went from more 
global to less global e.g. driver->device->client->keys which is now not 
for IMO no strong reason.

And if you add the "/* optional */" comment then why would only one of 
the optional keys "deserve" it.

I see you stormed it in already but as it doesn't harm much meh.

Regards,

Tvrtko

> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   lib/igt_drm_fdinfo.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> index 5c0ccf624..17cac4009 100644
> --- a/lib/igt_drm_fdinfo.c
> +++ b/lib/igt_drm_fdinfo.c
> @@ -233,12 +233,13 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>   		if ((v = find_kv(l, "drm-driver", strlen("drm-driver")))) {
>   			strncpy(info->driver, v, sizeof(info->driver) - 1);
>   			good++;
> -		} else if ((v = find_kv(l, "drm-pdev", strlen("drm-pdev")))) {
> -			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
>   		}  else if ((v = find_kv(l, "drm-client-id",
>   					 strlen("drm-client-id")))) {
>   			info->id = atol(v);
>   			good++;
> +		} else if ((v = find_kv(l, "drm-pdev", strlen("drm-pdev")))) {
> +			/* optional */
> +			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
>   		} else if (!strncmp(l, "drm-engine-capacity-", 20)) {
>   			idx = parse_engine(l, info,
>   					   strlen("drm-engine-capacity-"),