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 _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit