[igt-dev] [PATCH i-g-t v3 1/3] tests/i915/gem_mmap_gtt: Add invalid parameters test

[Chris Wilson]

Quoting Antonio Argenziano (2019-03-14 20:21:31)
> Add a test for an invalid handle being passed to the IOCTL.
> 
> v2:
>         - Expand test space. (Chris)
> 
> v3:
>         - Add local IOCTL wrapper. (Chris)
> 
> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  tests/i915/gem_mmap_gtt.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
> index f6fbbe19..237308a2 100644
> --- a/tests/i915/gem_mmap_gtt.c
> +++ b/tests/i915/gem_mmap_gtt.c
> @@ -821,6 +821,23 @@ run_without_prefault(int fd,
>         igt_enable_prefault();
>  }
>  
> +static int
> +err_gem_mmap_gtt(int i915, uint32_t handle, uint64_t size, uint64_t offset)
> +{
> +       struct drm_i915_gem_mmap arg = {
> +                               .handle = handle,
> +                               .size = size,
> +                               .offset = offset

Just one tab indent required.

> +       };
> +       int err = 0;
> +
> +       if(ioctl(i915, DRM_IOCTL_I915_GEM_MMAP_GTT, &arg))
            ^ space before (

Just give these a once over with the magic whitespace pen.

Later you have a mixture of
	{4096}
	{4096 }
	{ 4096}
	{ 2* 4096 }
etc which is catching my ocd.

> +               err = -errno;
Petri had to add
		igt_assume(err);
to this style to satisfy static analyzers.

> +
> +       errno = 0;
> +       return err;
> +}
> +
>  int fd;
>  
>  igt_main
> @@ -831,6 +848,25 @@ igt_main
>         igt_fixture
>                 fd = drm_open_driver(DRIVER_INTEL);
>  
> +       igt_subtest("bad-object") {
> +               int ret;
> +

Why the newline?

> +               uint32_t real_handle = gem_create(fd, 4096);
> +               uint32_t handles[20];
> +               int i = 0;
> +
> +               handles[i++] = 0xdeadbeef;
> +               for(int bit = 0; bit < 16; bit++)
> +                       handles[i++] = real_handle | (1 << (bit + 16));
> +               handles[i] = real_handle + 1;
> +
> +               for (; i < 0; i--) {
> +                       igt_debug("Trying to mmap_gtt invalid handle: %x.\n", handles[i]);
> +                       ret = err_gem_mmap_gtt(fd, handles[i], 0, 4096);
> +                       igt_assert_eq(ret, -ENOENT);

igt_assert_f(err_gem_mmap_gtt(fd, handles[i], 0, 4096) == -ENOENT,
             "Succeeded in trying to mmap_gtt invalid handle %x!\n,
	     handles[i]);
Maybe?

Provoke it to fail and see what you think gives the right amount of
information with least noise.

Anyway, series looks good to me, so
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

We just need to land the fixes first, and hope nothing breaks by being
fixed.
-Chris