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

[Emil Velikov]

On 15 September 2017 at 16:34, Kyle Brenneman <kbrenneman at nvidia.com> wrote:
> 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.
>
Thanks for the confirmation Kyle. Moved 3/4 to the start of the series
and pushed the lot.
We'll worry about making the _eglError messages sane at another point ;-)

-Emil