On Tue, Dec 2, 2014 at 1:35 PM, Mike Holmes <[email protected]> wrote:
> > > On 2 December 2014 at 06:27, Taras Kondratiuk <[email protected] > > wrote: > >> ODP application may want to override default ODP logging behaviour >> and use custom logging function. Add a weak odp_override_log() function >> for this purpose instead of default fprintf(). >> >> Signed-off-by: Taras Kondratiuk <[email protected]> >> --- >> platform/linux-generic/Makefile.am | 3 +- >> platform/linux-generic/include/api/odp_debug.h | 41 >> ++++++++++++++++-------- >> platform/linux-generic/odp_weak.c | 23 +++++++++++++ >> 3 files changed, 53 insertions(+), 14 deletions(-) >> create mode 100644 platform/linux-generic/odp_weak.c >> >> diff --git a/platform/linux-generic/Makefile.am >> b/platform/linux-generic/Makefile.am >> index e709700..cc78de3 100644 >> --- a/platform/linux-generic/Makefile.am >> +++ b/platform/linux-generic/Makefile.am >> @@ -75,4 +75,5 @@ __LIB__libodp_la_SOURCES = \ >> odp_thread.c \ >> odp_ticketlock.c \ >> odp_time.c \ >> - odp_timer.c >> + odp_timer.c \ >> + odp_weak.c >> diff --git a/platform/linux-generic/include/api/odp_debug.h >> b/platform/linux-generic/include/api/odp_debug.h >> index e853be4..4b51038 100644 >> --- a/platform/linux-generic/include/api/odp_debug.h >> +++ b/platform/linux-generic/include/api/odp_debug.h >> @@ -14,6 +14,7 @@ >> >> #include <stdio.h> >> #include <stdlib.h> >> +#include <stdarg.h> >> >> #ifdef __cplusplus >> extern "C" { >> @@ -81,61 +82,75 @@ typedef enum odp_log_level { >> } odp_log_level_e; >> >> /** >> - * ODP default LOG macro. >> + * ODP log function >> + * >> + * Instead of direct prints to stdout/stderr all logging in ODP >> implementation >> + * should be done via this function or its wrappers. >> + * ODP platform MUST provide a default *weak* implementation of this >> function. >> > > MUST -> must > > >> + * Application MAY override the function if needed by providing a strong >> > > MAY -> may > > I disagree. The use of CAPS indicates that RFC 2119 applies. It is a very standard convention and should be encouraged when applicable. > + * function. >> + * >> + * @param level Log level >> > > doxygen in or out > > >> + * @param fmt printf-style message format >> > > doxygen in or out > > >> + */ >> > > need @return description or if possible @retval if there are specific > cases > > >> +extern int odp_override_log(odp_log_level_e level, const char *fmt, ...); >> > > level does not appear to be the best name, these are output streams with > different characteristics rather than a single stream with different levels > > >> + >> +/** >> + * ODP LOG macro. >> */ >> #define ODP_LOG(level, fmt, ...) \ >> do { \ >> switch (level) { \ >> case ODP_LOG_ERR: \ >> - fprintf(stderr, "%s:%d:%s():" fmt, __FILE__, \ >> + odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \ >> __LINE__, __func__, ##__VA_ARGS__); \ >> break; \ >> case ODP_LOG_DBG: \ >> if (ODP_DEBUG_PRINT == 1) \ >> - fprintf(stderr, "%s:%d:%s():" fmt, __FILE__, \ >> + odp_override_log(level, "%s:%d:%s():" fmt, >> __FILE__, \ >> __LINE__, __func__, ##__VA_ARGS__); \ >> break; \ >> case ODP_LOG_PRINT: \ >> - fprintf(stdout, " " fmt, ##__VA_ARGS__); \ >> + odp_override_log(level, " " fmt, ##__VA_ARGS__); \ >> break; \ >> case ODP_LOG_ABORT: \ >> - fprintf(stderr, "%s:%d:%s(): " fmt, __FILE__, \ >> + odp_override_log(level, "%s:%d:%s(): " fmt, __FILE__, \ >> __LINE__, __func__, ##__VA_ARGS__); \ >> abort(); \ >> break; \ >> case ODP_LOG_UNIMPLEMENTED: \ >> - fprintf(stderr, \ >> + odp_override_log(level, \ >> "%s:%d:The function %s() is not implemented\n" \ >> fmt, __FILE__, __LINE__, __func__, >> ##__VA_ARGS__); \ >> break; \ >> default: \ >> - fprintf(stderr, "Unknown LOG level"); \ >> + odp_override_log(level, "Unknown LOG level"); \ >> break;\ >> } \ >> } while (0) >> >> /** >> - * Printing macro, which prints output when the application >> - * calls one of the ODP APIs specifically for dumping internal data. >> + * Log print message when the application calls one of the ODP APIs >> + * specifically for dumping internal data. >> */ >> #define ODP_PRINT(fmt, ...) \ >> ODP_LOG(ODP_LOG_PRINT, fmt, ##__VA_ARGS__) >> >> /** >> - * Debug printing macro, which prints output when DEBUG flag is set. >> + * Log debug message if DEBUG flag is set. >> > > The flag is ODP_DEBUG_PRINT > > >> */ >> #define ODP_DBG(fmt, ...) \ >> ODP_LOG(ODP_LOG_DBG, fmt, ##__VA_ARGS__) >> >> /** >> - * Print output to stderr (file, line and function). >> + * Log error message. >> > > We should say that this is not maskable > > >> */ >> #define ODP_ERR(fmt, ...) \ >> ODP_LOG(ODP_LOG_ERR, fmt, ##__VA_ARGS__) >> >> /** >> - * Print output to stderr (file, line and function), >> - * then abort. >> + * Log abort message and then stop execution (by default call abort()). >> + * This function should not return. >> */ >> #define ODP_ABORT(fmt, ...) \ >> ODP_LOG(ODP_LOG_ABORT, fmt, ##__VA_ARGS__) >> diff --git a/platform/linux-generic/odp_weak.c >> b/platform/linux-generic/odp_weak.c >> new file mode 100644 >> index 0000000..fccbc3f >> --- /dev/null >> +++ b/platform/linux-generic/odp_weak.c >> @@ -0,0 +1,23 @@ >> +/* Copyright (c) 2014, Linaro Limited >> + * All rights reserved. >> + * >> + * SPDX-License-Identifier: BSD-3-Clause >> + */ >> + >> +#include <odp_internal.h> >> +#include <odp_debug.h> >> +#include <odp_debug_internal.h> >> +#include <odp_hints.h> >> + >> +ODP_WEAK_SYMBOL int odp_override_log(odp_log_level_e level ODP_UNUSED, >> + const char *fmt, ...) >> > > Why is this ODP_UNUSED, is it needed ? > To use it a reimplementation of this function by an application would > effectively have to have another switch statement. > Generating that need for two switches implies we should be exporting > overrides for each log type and not > prematurely factorising our code. > > If we do that we just remove our switch statement and put the > implementation under the weak function that each macro directly calls. > > I think Zoltan is bringing this up on the other patch as well > > > >> +{ >> + va_list args; >> + int r; >> + >> + va_start(args, fmt); >> + r = vfprintf(stderr, fmt, args); >> + va_end(args); >> + >> + return r; >> +} >> -- >> 1.7.9.5 >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp > >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
