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

[Emil Velikov]

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