Re: [igt-dev] [PATCH i-g-t 3/9] tests/kms_lease: invalid corner-cases for create-lease ioctl

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Feb 20, 2019 at 07:08:30PM -0800, Keith Packard wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> 
> > Found a few things in the kernel that looks suspicious, separate
> > patches on their way.
> 
> Thanks for adding more tests!
> 
> > +	/* empty lease */
> > +	mcl.object_ids = 0;
> > +	mcl.object_count = 0;
> > +	mcl.flags = 0;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> 
> Not sure why this should be invalid? It's not useful, but then neither
> are 0x0 windows and look at all the pain not allowing them has caused
> over the years...
> 
> > +
> > +	/* NULL array pointer */
> > +	mcl.object_count = 1;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EFAULT);
> 
> Should probably explicitly initialize the array pointer here, avoids any
> dependency on the previous test.
> 
> > +	/* no crtc, non-universal_plane */
> > +	drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> > +	object_ids[0] = data->master.display.outputs[0].id;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> 
> Similarly, initialize the object count and pointers explicitly.
> 
> > +	/* no subleasing */
> > +	mcl.object_count = 3;
> > +	mcl.flags = 0;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), 0);
> > +	tmp_fd = mcl.fd;
> > +	igt_assert_eq(create_lease(tmp_fd, &mcl), -EINVAL);
> > +	close(tmp_fd);
> 
> Just reminds me that we should probably enable sub-leasing -- useful in
> a multi-head environment where you also want to run leased applications
> under one of the heads.
> 
> All of these tests look OK to me; as usual, the precise errno values are
> subject to interpretation, but it looks like you've made reasonable
> choices here making me wonder how reasonable the kernel is :-)

All the test in the current series here reflect current kernel behaviour.
So I didn't pick the errno at all, it's all the kernel. And some took some
amount of digging until I've figured out where they're from.

Hence why I check for NULL/0 in the object_count/ids, later checks making
sure there's no subleasing, and the choice of errno.

> Reviewed-by: Keith Packard <keithp@keithp.com>

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev