On 15 September 2017 at 16:34, Kyle Brenneman <kbrenne...@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.veli...@gmail.com> >> wrote: >>> >>> On 7 September 2017 at 19:21, Kyle Brenneman <kbrenne...@nvidia.com> >>> wrote: >>>> >>>> On 09/07/2017 11:56 AM, Emil Velikov wrote: >>>>> >>>>> On 7 September 2017 at 18:36, Kyle Brenneman <kbrenne...@nvidia.com> >>>>> wrote: >>>>>> >>>>>> On 09/07/2017 10:03 AM, Emil Velikov wrote: >>>>>>> >>>>>>> From: Emil Velikov <emil.veli...@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 <kbrenne...@nvidia.com> >>>>>>> Cc: Adam Jackson <a...@redhat.com> >>>>>>> Signed-off-by: Emil Velikov <emil.veli...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev