Re: [PATCH i-g-t 05/39] lib/vkms: Allow to enable/disable VKMS devices

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 18/02/2025 à 17:49, José Expósito a écrit :
> Add helper to enable, disable and check the enabled status of a VKMS
> device and add a test to ensure that the default value is correctly set.
> 
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>   lib/igt_vkms.c             | 80 ++++++++++++++++++++++++++++++++++++++
>   lib/igt_vkms.h             |  5 +++
>   tests/vkms/vkms_configfs.c | 18 +++++++++
>   3 files changed, 103 insertions(+)
> 
> diff --git a/lib/igt_vkms.c b/lib/igt_vkms.c
> index 8cea94901..1fdfb09f7 100644
> --- a/lib/igt_vkms.c
> +++ b/lib/igt_vkms.c
> @@ -7,7 +7,12 @@
>   
>   #include <dirent.h>
>   #include <errno.h>
> +#include <fcntl.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
>   #include <string.h>
> +#include <unistd.h>
>   
>   #include <ftw.h>
>   #include <linux/limits.h>
> @@ -17,6 +22,7 @@
>   #include "igt_vkms.h"
>   
>   #define VKMS_ROOT_DIR_NAME		"vkms"
> +#define VKMS_FILE_ENABLED		"enabled"
>   
>   /**
>    * SECTION:igt_vkms
> @@ -45,6 +51,48 @@ static const char *mount_vkms_configfs(void)
>   	return vkms_root_path;
>   }
>   
> +static int read_int(const char *path)
> +{
> +	FILE *file;
> +	int value;
> +	int ret;
> +
> +	file = fopen(path, "r");
> +	igt_assert_f(file, "Error opening '%s'\n", path);
> +
> +	ret = fscanf(file, "%d", &value);
> +	fclose(file);
> +	igt_assert_f(ret == 1, "Error reading integer from '%s'\n", path);
> +
> +	return value;
> +}
> +
> +static bool read_bool(const char *path)
> +{
> +	int ret;
> +
> +	ret = read_int(path);
> +
> +	return !!ret;
> +}
> +
> +static void write_int(const char *path, int value)
> +{
> +	FILE *file;
> +
> +	file = fopen(path, "w");
> +	igt_assert_f(file, "Error opening '%s'\n", path);
> +
> +	fprintf(file, "%d", value);

No check about the result of fprintf? This may hide some strange issues.

And in 09/39, the function igt_vkms_plane_set_type is broken because of 
this, it can fails silently.

> +
> +	fclose(file);
> +}
> +
> +static void write_bool(const char *path, bool value)
> +{
> +	write_int(path, value ? 1 : 0);
> +}
> +
>   /**
>    * igt_require_vkms_configfs:
>    *
> @@ -151,6 +199,8 @@ void igt_vkms_device_destroy(igt_vkms_t *dev)
>   
>   	igt_assert(dev);
>   
> +	igt_vkms_device_set_enabled(dev, false);
> +
>   	ret = remove_device_dir(dev);
>   	igt_assert_f(ret == 0,
>   		     "Unable to rmdir device directory '%s'. Got errno=%d (%s)\n",
> @@ -189,3 +239,33 @@ void igt_vkms_destroy_all_devices(void)
>   
>   	closedir(dir);
>   }
> +
> +/**
> + * igt_vkms_device_is_enabled:
> + * @dev: The device to check
> + *
> + * Indicate whether a VKMS device is enabled or not.
> + */
> +bool igt_vkms_device_is_enabled(igt_vkms_t *dev)
> +{
> +	char path[PATH_MAX];
> +
> +	snprintf(path, sizeof(path), "%s/%s", dev->path, VKMS_FILE_ENABLED);

As nothing garantee that dev->path is shorter than PATH_MAX, can you add 
a simple check:

igt_assert_le(snprintf(...), PATH_MAX);

> +	return read_bool(path);
> +}
> +
> +/**
> + * igt_vkms_device_set_enabled:
> + * @dev: Device to enable or disable
> + *
> + * Enable or disable a VKMS device.
> + */
> +void igt_vkms_device_set_enabled(igt_vkms_t *dev, bool enabled)
> +{
> +	char path[PATH_MAX];
> +
> +	snprintf(path, sizeof(path), "%s/%s", dev->path, VKMS_FILE_ENABLED);

Ditto

> +	write_bool(path, enabled);
> +}
> diff --git a/lib/igt_vkms.h b/lib/igt_vkms.h
> index 48c48f296..dab51852a 100644
> --- a/lib/igt_vkms.h
> +++ b/lib/igt_vkms.h
> @@ -8,6 +8,8 @@
>   #ifndef __IGT_VKMS_H__
>   #define __IGT_VKMS_H__
>   
> +#include <stdbool.h>
> +
>   /**
>    * igt_vkms_t:
>    * @path: VKMS root directory inside configfs mounted directory
> @@ -24,4 +26,7 @@ igt_vkms_t *igt_vkms_device_create(const char *name);
>   void igt_vkms_device_destroy(igt_vkms_t *dev);
>   void igt_vkms_destroy_all_devices(void);
>   
> +bool igt_vkms_device_is_enabled(igt_vkms_t *dev);
> +void igt_vkms_device_set_enabled(igt_vkms_t *dev, bool enabled);
> +
>   #endif /* __IGT_VKMS_H__ */
> diff --git a/tests/vkms/vkms_configfs.c b/tests/vkms/vkms_configfs.c
> index 441c39a47..6c7d0310c 100644
> --- a/tests/vkms/vkms_configfs.c
> +++ b/tests/vkms/vkms_configfs.c
> @@ -94,6 +94,23 @@ static void test_device_default_files(void)
>   	igt_vkms_device_destroy(dev);
>   }
>   
> +/**
> + * SUBTEST: device-default-values
> + * Description: Check that the default values for the device are correct.
> + */
> +
> +static void test_device_default_values(void)
> +{
> +	igt_vkms_t *dev;
> +
> +	dev = igt_vkms_device_create(__func__);
> +	igt_assert(dev);
> +
> +	igt_assert(!igt_vkms_device_is_enabled(dev));
> +
> +	igt_vkms_device_destroy(dev);
> +}
> +
>   igt_main
>   {
>   	struct {
> @@ -101,6 +118,7 @@ igt_main
>   		void (*fn)(void);
>   	} tests[] = {
>   		{ "device-default-files", test_device_default_files },
> +		{ "device-default-values", test_device_default_values },
>   	};
>   
>   	igt_fixture {

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com