[Mesa-dev] [PATCH 1/4] egl: simplify _eglDebugReport* API

[Kyle Brenneman]

On 09/15/2017 07:42 AM, Emil Velikov wrote:
> Hi Kyle,
>
> On 8 September 2017 at 13:09, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 7 September 2017 at 19:21, Kyle Brenneman <kbrenneman at nvidia.com> wrote:
>>> On 09/07/2017 11:56 AM, Emil Velikov wrote:
>>>> On 7 September 2017 at 18:36, Kyle Brenneman <kbrenneman at nvidia.com>
>>>> wrote:
>>>>> On 09/07/2017 10:03 AM, Emil Velikov wrote:
>>>>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>>>>
>>>>>> Instead of having three, almost identical but not quite,
>>>>>> _eglDebugReport* functions, simply fold them into one.
>>>>>>
>>>>>> While doing so drop the unnecessary arguments 'command' and
>>>>>> 'objectLabel'. Former is identical to funcName, while the latter is
>>>>>> already stored (yet unused) in _EGLThreadInfo::CurrentObjectLabel.
>>>>>>
>>>>>> Cc: Kyle Brenneman <kbrenneman at nvidia.com>
>>>>>> Cc: Adam Jackson <ajax at redhat.com>
>>>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>>>> ---
>>>>>>     src/egl/main/eglapi.c     |  3 +--
>>>>>>     src/egl/main/eglcurrent.c | 53
>>>>>> ++++++++++++-----------------------------------
>>>>>>     src/egl/main/eglcurrent.h |  7 -------
>>>>>>     3 files changed, 14 insertions(+), 49 deletions(-)
>>>>>>
>>>>> The command and funcName parameters aren't always identical. funcName is
>>>>> whatever string got passed to _eglError, which is often the name of some
>>>>> internal function. For instance, eglCreateWindowSurface can call
>>>>> _eglError
>>>>> with the string "_eglCreateWindowSurfaceCommon". A lot of the calls in
>>>>> the
>>>>> src/egl/drivers/ directory also use internal function names.
>>>>>
>>>>> I had both the command and fullName parameters in order to preserve the
>>>>> existing error reporting behavior (in _eglInternalError) in addition to
>>>>> the
>>>>> debug callback.
>>>>>
>>>>> Also, the reason I kept the command parameter at all instead of always
>>>>> using
>>>>> thr->CurrentFuncName is so that _eglSetFuncName could correctly report an
>>>>> EGL_BAD_ALLOC error if it couldn't allocate the _EGLThreadInfo struct in
>>>>> the
>>>>> first place. In that case, (thr->CurrentFuncName) would be NULL.
>>>>>
>>>>> You could change _eglError to pass NULL for the command name, in which
>>>>> case
>>>>> the debug callback would get the correct name in all cases, but then
>>>>> you'd
>>>>> lose the internal function name that would otherwise get passed to
>>>>> _eglLog.
>>>> I agree on the _eglError front - it is a bit iffy, as described in 3/4
>>>> [1].
>>>> At the same time I cannot find an instance where there will be a
>>>> difference.
>>>>
>>>> Can you point me to one?
>>>>
>>> Ah, I hadn't read patch 3/4 yet when I typed my reply.
>>>
>>> Without patch 3, if the app calls eglCreateWindowSurface with NULL for the
>>> native window handle, then you'd run into that. eglCreateWindowSurface calls
>>> _eglCreateWindowSurfaceCommon, which calls the RETURN_EGL_ERROR macro if it
>>> hits an error, which in turn would pass the string
>>> "eglCreateWindowSurfaceCommon" to _eglError, and from there to the callback.
>>>
>>> Changing _eglError like you did in patch 3 would avoid that problem.
>>>
>> Right, my bad. I'll swap the patch order.
>>
>>> However, I'm not sure how useful an internal function name by itself would
>>> be for an application developer, which is generally who the debug messages
>>> are aimed at.
>>>
>> Here is where the wonder of open-source comes handy :-P
>> The message makes it easier to track and resolve the problem... say
>> you cannot report the issue (for whatever reason).
>>
>> Overall I agree though, messages kind of suck. Yet again that's seems
>> orthogonal to what the series does.
>>
>>> Also, if you do use the msg parameter in eglError as the debug callback
>>> message, then it should probably use (..., "%s", msg), on the off-chance
>>> that some message ever has a printf character in it.
>>>
>>>
>> As/if we're to beat sense into _eglError "%s" would be very useful.
>> ATM nobody misuses the argument so we are safe.
>>
> With the above in mind, can you suggest any changes that I should make
> to the patch?
> Alternatively can you please review/ack the patch, please.
>
> Thanks
> Emil
If patch 3 gets moved to the beginning, then the set looks correct to 
me. I'd still recommend changing it to use a "%s" while you're at it, 
but I'll leave that to your discretion. It's not immediately needed, but 
it's an easy change and one less thing to go wrong in the future.

-Kyle