Re: [PATCH i-g-t v3 16/41] lib/vkms: Test connector invalid values

["José Expósito" <jose.exposito89@gmail.com>]

Hi again Louis,

On Wed, Jul 16, 2025 at 11:41:56AM +0200, Louis Chauvet wrote:
> 
> 
> Le 15/07/2025 à 12:24, José Expósito a écrit :
> > For a VKMS connector, it is only possible to set invalid values in the
> > connector status.
> > 
> > Test that setting wrong values fails and that the connector is not
> > accidentally changed.
> > 
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> >   lib/igt_vkms.c             | 24 ++++++++++++++++++
> >   lib/igt_vkms.h             |  2 ++
> >   tests/vkms/vkms_configfs.c | 50 ++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 76 insertions(+)
> > 
> > diff --git a/lib/igt_vkms.c b/lib/igt_vkms.c
> > index c8b0dd071..1cd560038 100644
> > --- a/lib/igt_vkms.c
> > +++ b/lib/igt_vkms.c
> > @@ -614,3 +614,27 @@ int igt_vkms_connector_get_status(igt_vkms_t *dev, const char *name)
> >   	return read_int(path);
> >   }
> > +
> > +/**
> > + * igt_vkms_connector_set_status:
> > + * @dev: Device the connector belongs to
> > + * @name: Connector name
> > + * @type: DRM_MODE_CONNECTED, DRM_MODE_DISCONNECTED or
> > + * DRM_MODE_UNKNOWNCONNECTION
> > + *
> > + * Set a new status for the connector
> > + */
> > +void igt_vkms_connector_set_status(igt_vkms_t *dev, const char *name,
> > +				   int status)
> > +{
> > +	char path[PATH_MAX];
> > +
> > +	if (status != DRM_MODE_CONNECTED &&
> > +	    status != DRM_MODE_DISCONNECTED &&
> > +	    status != DRM_MODE_UNKNOWNCONNECTION)
> > +		igt_assert(!"Cannot be reached: Unknown connector status");
> 
> Do we really need this? It is already checked by vkms.

We do not *need* it, but, since we shouldn't use other values in
our tests, it can help find bugs during development.

I think it is nice to have, but I don't oppose removing it if we
really don't like it.

Jose
 
> > +	igt_vkms_get_connector_status_path(dev, name, path, sizeof(path));
> > +
> > +	write_int(path, status);
> > +}
> > diff --git a/lib/igt_vkms.h b/lib/igt_vkms.h
> > index 3d27bfebb..df64bde9d 100644
> > --- a/lib/igt_vkms.h
> > +++ b/lib/igt_vkms.h
> > @@ -58,5 +58,7 @@ void igt_vkms_device_add_encoder(igt_vkms_t *dev, const char *name);
> >   void igt_vkms_device_add_connector(igt_vkms_t *dev, const char *name);
> >   int igt_vkms_connector_get_status(igt_vkms_t *dev, const char *name);
> > +void igt_vkms_connector_set_status(igt_vkms_t *dev, const char *name,
> > +				   int status);
> >   #endif /* __IGT_VKMS_H__ */
> > diff --git a/tests/vkms/vkms_configfs.c b/tests/vkms/vkms_configfs.c
> > index d7ce4aa17..c74bbc15f 100644
> > --- a/tests/vkms/vkms_configfs.c
> > +++ b/tests/vkms/vkms_configfs.c
> > @@ -486,6 +486,55 @@ static void test_connector_default_values(void)
> >   	igt_vkms_device_destroy(dev);
> >   }
> > +/**
> > + * SUBTEST: connector-wrong-values
> > + * Description: Check that setting unexpected values doesn't work.
> > + */
> > +
> > +static void test_connector_wrong_values(void)
> > +{
> > +	struct invalid_value invalid_status_values[] = {
> > +		{ "", 0 },
> > +		{ "\0", 1 },
> > +		{ "-1", 2 },
> > +		{ "0", 1 },
> > +		{ "4", 1 },
> > +		{ "connected", 10 },
> > +	};
> > +	igt_vkms_t *dev;
> > +	char path[PATH_MAX];
> > +	int fd;
> > +	int ret;
> > +
> > +	/* Create a device with a disconnected connector */
> > +	dev = igt_vkms_device_create(__func__);
> > +	igt_assert(dev);
> > +
> > +	igt_vkms_device_add_connector(dev, "connector0");
> > +	igt_vkms_connector_set_status(dev, "connector0", DRM_MODE_DISCONNECTED);
> > +	igt_assert_eq(igt_vkms_connector_get_status(dev, "connector0"),
> > +		      DRM_MODE_DISCONNECTED);
> > +	igt_vkms_get_connector_status_path(dev, "connector0", path, sizeof(path));
> > +
> > +	/* Test invalid values for "status" */
> > +	for (int i = 0; i < ARRAY_SIZE(invalid_status_values); i++) {
> > +		struct invalid_value v = invalid_status_values[i];
> > +
> > +		fd = open(path, O_WRONLY);
> > +		igt_assert_f(fd >= 0, "Error opening '%s'\n", path);
> > +
> > +		ret = write(fd, v.value, v.size);
> > +		igt_assert(ret <= 0);
> > +
> > +		close(fd);
> > +	}
> > +
> > +	igt_assert_eq(igt_vkms_connector_get_status(dev, "connector0"),
> > +		      DRM_MODE_DISCONNECTED);
> > +
> > +	igt_vkms_device_destroy(dev);
> > +}
> > +
> >   igt_main
> >   {
> >   	struct {
> > @@ -506,6 +555,7 @@ igt_main
> >   		{ "encoder-default-files", test_encoder_default_files },
> >   		{ "connector-default-files", test_connector_default_files },
> >   		{ "connector-default-values", test_connector_default_values },
> > +		{ "connector-wrong-values", test_connector_wrong_values },
> >   	};
> >   	igt_fixture {
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>