[Mesa-dev] [PATCH 13/13] mesa/main: Verify context creation on progress

[Pohjolainen, Topi]

On Fri, May 08, 2015 at 03:23:52PM +0300, Juha-Pekka Heikkil? wrote:
>    perjantai 8. toukokuuta 2015 Ian Romanick <idr at freedesktop.org> kirjoitti:
> 
>      On 05/07/2015 05:21 AM, Pohjolainen, Topi wrote:
>      > On Tue, May 05, 2015 at 02:25:29PM +0300, Juha-Pekka Heikkila wrote:
>      >> Stop context creation if something failed. If something errored
>      >> during context creation we'd segfault. Now will clean up and
>      >> return error.
>      >>
>      >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>      >> ---
>      >>  src/mesa/main/shared.c | 66
>      +++++++++++++++++++++++++++++++++++++++++++++++---
>      >>  1 file changed, 62 insertions(+), 4 deletions(-)
>      >>
>      >> diff --git a/src/mesa/main/shared.c b/src/mesa/main/shared.c
>      >> index 0b76cc0..cc05b05 100644
>      >> --- a/src/mesa/main/shared.c
>      >> +++ b/src/mesa/main/shared.c
>      >> @@ -64,9 +64,21 @@ _mesa_alloc_shared_state(struct gl_context *ctx)
>      >>
>      >>     mtx_init(&shared->Mutex, mtx_plain);
>      >>
>      >> +   /* Mutex and timestamp for texobj state validation */
>      >> +   mtx_init(&shared->TexMutex, mtx_recursive);
>      >> +   shared->TextureStateStamp = 0;
>      >
>      > Do you really need to move this here?
> 
>      I was going to ask the same thing.  I think moving it here means that it
>      can be unconditionally mtx_destroy'ed in the error path below.
> 
>    Yes, Ian got it correct. When moving mutex creation here there is no need
>    to go checking about it if need to clean up. I though this makes things
>    more clean and simple.

As it can't fail, I would have moved it right before the return in the
success path. Saves you from adding the mutex tear-down in the failure path.

>     
> 
>       
> 
>      >> +
>      >>     shared->DisplayList = _mesa_NewHashTable();
>      >> +   if (!shared->DisplayList)
>      >> +      goto error_out;
>      >> +
>      >>     shared->TexObjects = _mesa_NewHashTable();
>      >> +   if (!shared->TexObjects)
>      >> +      goto error_out;
>      >> +
>      >>     shared->Programs = _mesa_NewHashTable();
>      >> +   if (!shared->Programs)
>      >> +      goto error_out;
>      >>
>      >>     shared->DefaultVertexProgram =
>      >>        gl_vertex_program(ctx->Driver.NewProgram(ctx,
>      >> @@ -76,17 +88,28 @@ _mesa_alloc_shared_state(struct gl_context *ctx)
>      >>                                                 
>       GL_FRAGMENT_PROGRAM_ARB, 0));
>      >>
>      >>     shared->ATIShaders = _mesa_NewHashTable();
>      >> +   if (!shared->ATIShaders)
>      >> +      goto error_out;
>      >> +
>      >>     shared->DefaultFragmentShader =
>      _mesa_new_ati_fragment_shader(ctx, 0);
>      >>
>      >>     shared->ShaderObjects = _mesa_NewHashTable();
>      >> +   if (!shared->ShaderObjects)
>      >> +      goto error_out;
>      >>
>      >>     shared->BufferObjects = _mesa_NewHashTable();
>      >> +   if (!shared->BufferObjects)
>      >> +      goto error_out;
>      >>
>      >>     /* GL_ARB_sampler_objects */
>      >>     shared->SamplerObjects = _mesa_NewHashTable();
>      >> +   if (!shared->SamplerObjects)
>      >> +      goto error_out;
>      >>
>      >>     /* Allocate the default buffer object */
>      >>     shared->NullBufferObj = ctx->Driver.NewBufferObject(ctx, 0);
>      >> +   if (!shared->NullBufferObj)
>      >> +       goto error_out;
>      >>
>      >>     /* Create default texture objects */
>      >>     for (i = 0; i < NUM_TEXTURE_TARGETS; i++) {
>      >> @@ -107,22 +130,57 @@ _mesa_alloc_shared_state(struct gl_context
>      *ctx)
>      >>        };
>      >>        STATIC_ASSERT(ARRAY_SIZE(targets) == NUM_TEXTURE_TARGETS);
>      >>        shared->DefaultTex[i] = ctx->Driver.NewTextureObject(ctx, 0,
>      targets[i]);
>      >> +
>      >> +      if (!shared->DefaultTex[i])
>      >> +          goto error_out;
>      >>     }
>      >>
>      >>     /* sanity check */
>      >>     assert(shared->DefaultTex[TEXTURE_1D_INDEX]->RefCount == 1);
>      >>
>      >> -   /* Mutex and timestamp for texobj state validation */
>      >> -   mtx_init(&shared->TexMutex, mtx_recursive);
>      >> -   shared->TextureStateStamp = 0;
>      >> -
>      >>     shared->FrameBuffers = _mesa_NewHashTable();
>      >> +   if (!shared->FrameBuffers)
>      >> +      goto error_out;
>      >> +
>      >>     shared->RenderBuffers = _mesa_NewHashTable();
>      >> +   if (!shared->RenderBuffers)
>      >> +      goto error_out;
>      >>
>      >>     shared->SyncObjects = _mesa_set_create(NULL, _mesa_hash_pointer,
>      >>                                            _mesa_key_pointer_equal);
>      >> +   if (!shared->SyncObjects)
>      >> +      goto error_out;
>      >>
>      >>     return shared;
>      >> +
>      >> +error_out:
>      >> +   for (i = 0; i < NUM_TEXTURE_TARGETS; i++) {
>      >> +      if (shared->DefaultTex[i]) {
>      >> +         ctx->Driver.DeleteTexture(ctx, shared->DefaultTex[i]);
>      >> +      }
>      >> +   }
>      >> +
>      >> +   _mesa_reference_buffer_object(ctx, &shared->NullBufferObj, NULL);
>      >> +
>      >> +   _mesa_DeleteHashTable(shared->RenderBuffers);
>      >> +   _mesa_DeleteHashTable(shared->FrameBuffers);
>      >> +   _mesa_DeleteHashTable(shared->SamplerObjects);
>      >> +   _mesa_DeleteHashTable(shared->BufferObjects);
>      >> +   _mesa_DeleteHashTable(shared->ShaderObjects);
>      >> +   _mesa_DeleteHashTable(shared->ATIShaders);
>      >> +   _mesa_DeleteHashTable(shared->Programs);
>      >> +   _mesa_DeleteHashTable(shared->TexObjects);
>      >> +   _mesa_DeleteHashTable(shared->DisplayList);
>      >> +
>      >> +   _mesa_reference_vertprog(ctx, &shared->DefaultVertexProgram,
>      NULL);
>      >> +   _mesa_reference_geomprog(ctx, &shared->DefaultGeometryProgram,
>      NULL);
>      >> +   _mesa_reference_fragprog(ctx, &shared->DefaultFragmentProgram,
>      NULL);
>      >> +
>      >> +   mtx_destroy(&shared->Mutex);
>      >> +   mtx_destroy(&shared->TexMutex);
>      >> +
>      >> +   free(shared);
>      >> +   return NULL;
>      >>  }
>      >>
>      >>
>      >> --
>      >> 1.8.5.1
>      >>
>      >> _______________________________________________
>      >> mesa-dev mailing list
>      >> mesa-dev at lists.freedesktop.org
>      >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>      > _______________________________________________
>      > mesa-dev mailing list
>      > mesa-dev at lists.freedesktop.org
>      > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>      >