[Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon

[Eric Engestrom]

On Saturday, 2017-08-05 00:25:49 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> As mentioned in previous commit the negative tests in dEQP expect the
> arguments to be evaluated in particular order.

The spec doesn't say that, so the test is wrong.
Changing it in Mesa doesn't hurt though, so I have nothing against it,
except for the fact it hide the dEQP bug.

Does anyone know the preferred way to report dEQP bugs? The README [1]
is rather... bare.
I have about a dozen known dEQP bugs that I'd love to report, if only so
that I don't have to keep telling people "ignore that test, it's broken"
but instead point them at the report of why & how it's broken.

[1] https://android.googlesource.com/platform/external/deqp/+/master/README.md

> 
> Namely - first the dpy, then the config, followed by the surface/window.
> 
> Move the check further down or executing the test below will produce
> the following error.
> 
>    dEQP-EGL.functional.negative_api.create_pbuffer_surface
> 
> 
>    <Section Name="Test2" Description="EGL_BAD_CONFIG is generated if config is not an EGL frame buffer configuration">
>       <Text>eglCreateWindowSurface(0x9bfff0f150, 0xffffffffffffffff, 0x0000000000000000, { EGL_NONE });</Text>
>       <Text>// 0x0000000000000000 returned</Text>
>       <Text>// ERROR expected: EGL_BAD_CONFIG, Got: EGL_BAD_NATIVE_WINDOW</Text>
>    </Section>
> 
> Cc: <mesa-stable at lists.freedesktop.org>
> Cc: Mark Janes <mark.a.janes at intel.com>
> Cc: Chad Versace <chadversary at chromium.org>
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> Mark,
> 
> IMHO the CI does the impossible and passes the test. Perhaps it's worth
> looking into how/why it does so - I don't know.
> 
> I'll pipe the series through Jenkins tomorrow - don't want to stall
> things for the guys still working.
> 
> Chad, I see that in the EGL_MESA_surfaceless implementation you
> explicitly mentioned that the surface is checked prior to the config.
> 
> Wouldn't it be better to stay consistent and move those, as per the
> above? AFAICT the spec does not explicitly dictates the order.
> ---
>  src/egl/main/eglapi.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index 3ca3dd4c7c1..3b0f896f74c 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -872,10 +872,6 @@ _eglCreateWindowSurfaceCommon(_EGLDisplay *disp, EGLConfig config,
>     _EGLSurface *surf;
>     EGLSurface ret;
>  
> -
> -   if (native_window == NULL)
> -      RETURN_EGL_ERROR(disp, EGL_BAD_NATIVE_WINDOW, EGL_NO_SURFACE);
> -
>  #ifdef HAVE_SURFACELESS_PLATFORM
>     if (disp && disp->Platform == _EGL_PLATFORM_SURFACELESS) {
>        /* From the EGL_MESA_platform_surfaceless spec (v1):
> @@ -899,6 +895,9 @@ _eglCreateWindowSurfaceCommon(_EGLDisplay *disp, EGLConfig config,
>     if ((conf->SurfaceType & EGL_WINDOW_BIT) == 0)
>        RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_NO_SURFACE);
>  
> +   if (native_window == NULL)
> +      RETURN_EGL_ERROR(disp, EGL_BAD_NATIVE_WINDOW, EGL_NO_SURFACE);
> +
>     surf = drv->API.CreateWindowSurface(drv, disp, conf, native_window,
>                                         attrib_list);
>     ret = (surf) ? _eglLinkSurface(surf) : EGL_NO_SURFACE;
> -- 
> 2.13.3
>