[Piglit] [PATCH 1/2] egl/utils: Prepare egl_util_run to be called from piglit subtests.

[Sarah Sharp]

On Sat, Apr 19, 2014 at 12:20:26PM -0700, Chad Versace wrote:
> On Fri, Apr 18, 2014 at 03:37:38PM -0700, Sarah Sharp wrote:
> > -static void
> > +static enum piglit_result
> >  create_window(struct egl_state *state)
> >  {
> >  	XSetWindowAttributes window_attr;
> > @@ -111,14 +113,14 @@ create_window(struct egl_state *state)
> >  	if (!eglGetConfigAttrib(state->egl_dpy,
> >  				state->cfg, EGL_NATIVE_VISUAL_ID, &id)) {
> >  		fprintf(stderr, "eglGetConfigAttrib() failed\n");
> > -		piglit_report_result(PIGLIT_FAIL);
> > +		return PIGLIT_FAIL;
> >  	}
> >  
> >  	template.visualid = id;
> >  	vinfo = XGetVisualInfo(state->dpy, VisualIDMask, &template, &count);
> >  	if (count != 1) {
> >  		fprintf(stderr, "XGetVisualInfo() failed\n");
> 
> Memory leak here. XFree(vinfo) is needed.

Ok, I'll fix that.

> > @@ -256,40 +261,55 @@ egl_util_run(const struct egl_test *test, int argc, char *argv[])
> >  	if (!eglChooseConfig(state.egl_dpy, test->config_attribs, &state.cfg, 1, &count) ||
> >  	    count == 0) {
> >  		fprintf(stderr, "eglChooseConfig() failed\n");
> > -		piglit_report_result(PIGLIT_FAIL);
> > +		test->result = PIGLIT_FAIL;
> > +		goto fail;
> >  	}
> >  
> >  	state.ctx = eglCreateContext(state.egl_dpy, state.cfg,
> >  				     EGL_NO_CONTEXT, ctxAttribs);
> >  	if (state.ctx == EGL_NO_CONTEXT) {
> >  		fprintf(stderr, "eglCreateContext() failed\n");
> > -		piglit_report_result(PIGLIT_FAIL);
> > +		test->result = PIGLIT_FAIL;
> > +		goto fail;
> >  	}
> >  
> >  	state.width = test->window_width;
> >  	state.height = test->window_height;
> > -	create_window(&state);
> > +	test->result = create_window(&state);
> > +	if (test->result != PIGLIT_PASS)
> > +		goto destroy_ctx;
> 
> Please use braces for the above goto, so future Piglit doesn't repeat
> Apple's SSL goto fail.

Hah, sure!  I'm used to the kernel coding style where minimizing line
count is emphasized and removing extra braces is even encoded into
checkpatch.pl.

> >  	state.surf = eglCreateWindowSurface(state.egl_dpy,
> >  					    state.cfg, state.win, NULL);
> >  	if (state.surf == EGL_NO_SURFACE) {
> >  		fprintf(stderr, "eglCreateWindowSurface() failed\n");
> > -		piglit_report_result(PIGLIT_FAIL);
> > +		test->result = PIGLIT_FAIL;
> > +		goto destroy_window;
> >  	}
> >  
> >  	if (!eglMakeCurrent(state.egl_dpy,
> >  			    state.surf, state.surf, state.ctx)) {
> >  		fprintf(stderr, "eglMakeCurrent() failed\n");
> > -		piglit_report_result(PIGLIT_FAIL);
> > +		test->result = PIGLIT_FAIL;
> > +		goto destroy_window;
> >  	}
> >  
> >  	piglit_dispatch_default_init(dispatch_api);
> >  
> > -	result = event_loop(&state, test);
> > +	test->result = event_loop(&state, test);
> >  
> > +destroy_window:
> > +	glFinish();
> 
> There's no need to call glFinish() here. Unless you're working around
> a Mesa bug? If so, then please add a comment briefly explaining the
> driver bug and workaround.

I think that was leftover from when I was trying to figure out why the
X window created in the first subtest wasn't closing when the second
subtest was run.  Rob helped me find that bug, but I forgot to move the
glFinish() call.  The tests work without glFinish(), so I'll remove it.

> > +	eglMakeCurrent(state.egl_dpy, EGL_NO_SURFACE, EGL_NO_SURFACE, state.ctx);
> > +	XDestroyWindow(state.dpy, state.win);
> > +destroy_ctx:
> > +	eglMakeCurrent(state.egl_dpy, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
> > +	eglDestroyContext(state.egl_dpy, state.ctx);
> >  	eglTerminate(state.egl_dpy);
> > -
> > -	piglit_report_result(result);
> > -
> > -	return EXIT_SUCCESS;
> > +fail:
> > +	if (test->stop_on_failure)
> > +		piglit_report_result(test->result);
> > +	if (test->result == PIGLIT_PASS)
> > +		return EXIT_SUCCESS;
> > +	return EXIT_FAILURE;
> 
> I tried to convince myself that the above cleanup was correct, but I got
> dizzy. By consolidating the destroy_window and destroy_ctx into the fail
> label, I think you can achieve more concise code that's more easily
> verifiable.
> 
> fail:
>     if (state.egl_dpy) {
>         eglMakeCurrent(state.egl_dpy, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
>         eglTerminate(state.egl_dpy);
>     }
>     if (state.win)
>         XDestroyWindow(state.win);
>     if (test->stop_on_failure)
>     	piglit_report_result(test->result);
>     if (test->result == PIGLIT_PASS)
>     	return EXIT_SUCCESS;
>     return EXIT_FAILURE;

Ok, sure, I can do that to make the failure case more readable.

> >  }
> > diff --git a/tests/egl/egl-util.h b/tests/egl/egl-util.h
> > index e1caa94..a2b89d2 100644
> > --- a/tests/egl/egl-util.h
> > +++ b/tests/egl/egl-util.h
> > @@ -35,6 +35,8 @@ struct egl_test {
> >  	enum piglit_result (*draw)(struct egl_state *state);
> >  	EGLint window_width;
> >  	EGLint window_height;
> > +	bool stop_on_failure;
> > +	enum piglit_result result;
> >  };
> >  
> >  static const EGLint egl_default_attribs[] = {
> > @@ -60,7 +62,7 @@ EGLSurface
> >  egl_util_create_pixmap(struct egl_state *state,
> >  		       int width, int height, const EGLint *attribs);
> >  
> > -int egl_util_run(const struct egl_test *test, int argc, char *argv[]);
> > +int egl_util_run(struct egl_test *test, int argc, char *argv[]);
> 
> I really really want to keep the 'test' parameter const. Let's return
> the piglit_result as the function's return value or as an out-parameter.

It seems like changing the return value from int to EGLBoolean would be
the cleanest.  egl_util_run currently looks like it returns EXIT_SUCCESS
or EXIT_FAILURE, but piglit_report_result() will never return.  The EGL
tests all "return" the value from egl_util_run as the last part of their
main function, which I think is only there to stop the compiler
complaining about no return value.  So the code could change to:

EGLBoolean egl_util_run(struct egl_test *test, int argc, char *argv[]);

int
main(int argc, char *argv[])
{
        struct egl_test test;

        egl_init_test(&test);
        test.draw = draw;

        if (egl_util_run(&test, argc, argv) != PIGLIT_PASS)
                return EXIT_FAILURE;
        return EXIT_SUCCESS;
}

Sarah Sharp