[Mesa-dev] [PATCH 2/4] st/mesa: check return value of begin/end_query

[Samuel Pitoiset]

On 04/20/2016 05:43 PM, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> They can only indicate out of memory conditions, since the other error
> conditions are caught earlier.
> ---
>   src/mesa/state_tracker/st_cb_queryobj.c | 55 ++++++++++++++++++++-------------
>   1 file changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_queryobj.c b/src/mesa/state_tracker/st_cb_queryobj.c
> index cdb9efc..784ffde 100644
> --- a/src/mesa/state_tracker/st_cb_queryobj.c
> +++ b/src/mesa/state_tracker/st_cb_queryobj.c
> @@ -61,13 +61,9 @@ st_NewQueryObject(struct gl_context *ctx, GLuint id)
>   }
>
>
> -
>   static void
> -st_DeleteQuery(struct gl_context *ctx, struct gl_query_object *q)
> +free_queries(struct pipe_context *pipe, struct st_query_object *stq)
>   {
> -   struct pipe_context *pipe = st_context(ctx)->pipe;
> -   struct st_query_object *stq = st_query_object(q);
> -
>      if (stq->pq) {
>         pipe->destroy_query(pipe, stq->pq);
>         stq->pq = NULL;
> @@ -77,6 +73,16 @@ st_DeleteQuery(struct gl_context *ctx, struct gl_query_object *q)
>         pipe->destroy_query(pipe, stq->pq_begin);
>         stq->pq_begin = NULL;
>      }
> +}
> +
> +
> +static void
> +st_DeleteQuery(struct gl_context *ctx, struct gl_query_object *q)
> +{
> +   struct pipe_context *pipe = st_context(ctx)->pipe;
> +   struct st_query_object *stq = st_query_object(q);
> +
> +   free_queries(pipe, stq);
>
>      free(stq);
>   }
> @@ -89,6 +95,7 @@ st_BeginQuery(struct gl_context *ctx, struct gl_query_object *q)
>      struct pipe_context *pipe = st->pipe;
>      struct st_query_object *stq = st_query_object(q);
>      unsigned type;
> +   bool ret = false;
>
>      st_flush_bitmap_cache(st_context(ctx));
>
> @@ -133,14 +140,7 @@ st_BeginQuery(struct gl_context *ctx, struct gl_query_object *q)
>
>      if (stq->type != type) {
>         /* free old query of different type */
> -      if (stq->pq) {
> -         pipe->destroy_query(pipe, stq->pq);
> -         stq->pq = NULL;
> -      }
> -      if (stq->pq_begin) {
> -         pipe->destroy_query(pipe, stq->pq_begin);
> -         stq->pq_begin = NULL;
> -      }
> +      free_queries(pipe, stq);
>         stq->type = PIPE_QUERY_TYPES; /* an invalid value */
>      }
>
> @@ -151,20 +151,25 @@ st_BeginQuery(struct gl_context *ctx, struct gl_query_object *q)
>            stq->pq_begin = pipe->create_query(pipe, type, 0);
>            stq->type = type;
>         }
> -      pipe->end_query(pipe, stq->pq_begin);
> +      if (stq->pq_begin)
> +         ret = pipe->end_query(pipe, stq->pq_begin);
>      } else {
>         if (!stq->pq) {
>            stq->pq = pipe->create_query(pipe, type, q->Stream);
>            stq->type = type;
>         }
> -      if (stq->pq) {
> -         pipe->begin_query(pipe, stq->pq);
> -      }
> -      else {
> -         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBeginQuery");
> -         return;
> -      }
> +      if (stq->pq)
> +         ret = pipe->begin_query(pipe, stq->pq);
>      }
> +
> +   if (!ret) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBeginQuery");
> +
> +      free_queries(pipe, stq);

Why do you need to free queries now? Was there a memleak before? And why 
not in glEndQuery too?

> +      q->Active = GL_FALSE;
> +      return;
> +   }
> +
>      assert(stq->type == type);
>   }
>
> @@ -174,6 +179,7 @@ st_EndQuery(struct gl_context *ctx, struct gl_query_object *q)
>   {
>      struct pipe_context *pipe = st_context(ctx)->pipe;
>      struct st_query_object *stq = st_query_object(q);
> +   bool ret = false;
>
>      st_flush_bitmap_cache(st_context(ctx));
>
> @@ -185,7 +191,12 @@ st_EndQuery(struct gl_context *ctx, struct gl_query_object *q)
>      }
>
>      if (stq->pq)
> -      pipe->end_query(pipe, stq->pq);
> +      ret = pipe->end_query(pipe, stq->pq);
> +
> +   if (!ret) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBeginQuery");

This should be "glEndQuery", right? I think it would be better to move 
the error message inside the previous branch, what do you think?

> +      return;
> +   }
>   }
>
>
>