diff mbox

[PATCHv4,1/2] platform: debug: replace fprintf() with odp_override_log()

Message ID 1417519667-7902-2-git-send-email-taras.kondratiuk@linaro.org
State New
Headers show

Commit Message

Taras Kondratiuk Dec. 2, 2014, 11:27 a.m. UTC 
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 <taras.kondratiuk@linaro.org>
---
 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

Comments

Mike Holmes Dec. 2, 2014, 6:35 p.m. UTC | #1 
On 2 December 2014 at 06:27, Taras Kondratiuk <taras.kondratiuk@linaro.org>
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 <taras.kondratiuk@linaro.org>
> ---
>  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


> + * 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
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Bill Fischofer Dec. 2, 2014, 7:03 p.m. UTC | #2 
On Tue, Dec 2, 2014 at 1:35 PM, Mike Holmes <mike.holmes@linaro.org> wrote:

>
>
> On 2 December 2014 at 06:27, Taras Kondratiuk <taras.kondratiuk@linaro.org
> > 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 <taras.kondratiuk@linaro.org>
>> ---
>>  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
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
>
> --
> *Mike Holmes*
> Linaro  Sr Technical Manager
> LNG - ODP
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Anders Roxell Dec. 2, 2014, 7:13 p.m. UTC | #3 
On 2 December 2014 at 20:03, Bill Fischofer <bill.fischofer@linaro.org> wrote:
>
>
> On Tue, Dec 2, 2014 at 1:35 PM, Mike Holmes <mike.holmes@linaro.org> wrote:
>>
>>
>>
>> On 2 December 2014 at 06:27, Taras Kondratiuk
>> <taras.kondratiuk@linaro.org> 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 <taras.kondratiuk@linaro.org>
>>> ---
>>>  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.

I agree that we should say that in the ODP Specification (Architecture
doc), that we follow RFC 2119.
We have *not* done that in the API docs at all (use git grep).
If you have a couple of example API docs that uses RFC 2119 in the
text then we should think about doing the same.

Cheers,
Anders
Taras Kondratiuk Dec. 3, 2014, 8:31 a.m. UTC | #4 
On 12/02/2014 08:35 PM, Mike Holmes wrote:

>     - * 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

This follows our specification approach to use RFC 2119.

>
>     + * Application MAY override the function if needed by providing a
>     strong
>
>
> MAY -> may
>
>     + * 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

Will add.

>
>     +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

No. This is a level. It even has type of odp_log_level_e.
Mapping it to streams is an implementation/application choice.

>
>     +
>     +/**
>     + * 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

I didn't change the original flag name. Will update it.

>
>        */
>       #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

None of them is maskable except DEBUG level. Do you want to add
an explicit note to each of them?

>
>        */
>       #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 ?

Because level is unused in this default definition.

> 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.

Have you looked at the next patch (#2)? I especially haven't done those
changes in a first patch to keep it minimal.
Mike Holmes Dec. 3, 2014, 6:44 p.m. UTC | #5 
On 3 December 2014 at 03:31, Taras Kondratiuk <taras.kondratiuk@linaro.org>
wrote:

> On 12/02/2014 08:35 PM, Mike Holmes wrote:
>
>      - * 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
>>
>
> This follows our specification approach to use RFC 2119.



We need to clarify this, we have not used RFC 2119 upper case in any API to
date, and the api guideline doc
<http://docs.opendataplane.org/arch/html/api_guide_lines.html> does not
state we need to.

mike@fedora1:~/git/odp/platform/linux-generic/include/api$ git grep must
odp_classification.h: * @param[in]      offset          Number of bytes the
classifier must skip.
odp_classification.h: *                 that must match the value size
requirement of the
odp_classification.h: *                 that must match the value size
requirement of the
odp_init.h: * This function must be called once before calling any other
ODP API
odp_init.h: * @sa odp_term_local() which must have been called prior to
this.
odp_init.h: * All threads must call this function before calling
odp_init.h: * @sa odp_init_global() which must have been called prior to
this.
odp_init.h: * All threads must call this function before calling

mike@fedora1:~/git/odp/platform/linux-generic/include/api$ git grep may
odp_classification.h: * for fields that may be used to calculate
odp_classification.h: * for fields that may be used to calculate
odp_classification.h:   /** Inner header may repeat above values with this
offset */
odp_classification.h: * for fields that may be used to identify
odp_classification.h: * The underlying platform may not support all or any
specific combination
odp_classification.h: * @return                 Return value may be a
positive number
odp_classification.h: *                         may be in the range from 1
to num_terms,
odp_classification.h: * may not guarantee the availability of hardware
resources to create the
odp_init.h: * thread before the other ODP APIs may be called.
odp_init.h: * This api may have  platform dependant implications.
odp_packet.h: * frame, the protocol header may start 2 or 6 bytes within
the buffer to
odp_version.h: * Every implementation of ODP may receive bug fixes
independent of the version

I am OK with the change if we get a follow on patch to update all the API
to match and the API guide line doc gets a patch to specify this is how
things must be defined in the API headers.

I have added a note to
https://docs.google.com/document/d/17xVCXorgZ2KjmS7DiKwb2pp576XdUp1f1IXK_bCIUys/edit?usp=sharing



>
>
>
>>     + * Application MAY override the function if needed by providing a
>>     strong
>>
>>
>> MAY -> may
>>
>>     + * 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
>>
>
> Will add.
>
>
>>     +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
>>
>
> No. This is a level. It even has type of odp_log_level_e.
> Mapping it to streams is an implementation/application choice.
>
>
>
>>     +
>>     +/**
>>     + * 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
>>
>
> I didn't change the original flag name. Will update it.
>
>
>>        */
>>       #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
>>
>
> None of them is maskable except DEBUG level. Do you want to add
> an explicit note to each of them?
>
>
>
>>        */
>>       #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 ?
>>
>
> Because level is unused in this default definition.
>
>  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.
>>
>
> Have you looked at the next patch (#2)? I especially haven't done those
> changes in a first patch to keep it minimal.
>
diff mbox

Patch

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.
+ * Application MAY override the function if needed by providing a strong
+ * function.
+ *
+ * @param level   Log level
+ * @param fmt     printf-style message format
+ */
+extern int odp_override_log(odp_log_level_e level, const char *fmt, ...);
+
+/**
+ * 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.
  */
 #define ODP_DBG(fmt, ...) \
 		ODP_LOG(ODP_LOG_DBG, fmt, ##__VA_ARGS__)
 
 /**
- * Print output to stderr (file, line and function).
+ * Log error message.
  */
 #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, ...)
+{
+	va_list args;
+	int r;
+
+	va_start(args, fmt);
+	r = vfprintf(stderr, fmt, args);
+	va_end(args);
+
+	return r;
+}