| Message ID | 1417519667-7902-3-git-send-email-taras.kondratiuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, At the moment, the careless developer look at "enum odp_log_level" and starts using ODP_LOG(ODP_LOG_DBG, ...) instead of ODP_DBG. I think it would be good to provide either ODP_LOG(loglevel, ...) or ODP_[loglevel](...), but not both. Regards, Zoli On 02/12/14 11:27, Taras Kondratiuk wrote: > Move additional functionality out of ODP_LOG. Keep only logging. > > Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > --- > platform/linux-generic/include/api/odp_debug.h | 43 +++++--------------- > .../linux-generic/include/odp_debug_internal.h | 6 ++- > 2 files changed, 15 insertions(+), 34 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_debug.h b/platform/linux-generic/include/api/odp_debug.h > index 4b51038..13c3939 100644 > --- a/platform/linux-generic/include/api/odp_debug.h > +++ b/platform/linux-generic/include/api/odp_debug.h > @@ -99,48 +99,24 @@ 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: \ > - odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \ > - __LINE__, __func__, ##__VA_ARGS__); \ > - break; \ > - case ODP_LOG_DBG: \ > - if (ODP_DEBUG_PRINT == 1) \ > - odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \ > - __LINE__, __func__, ##__VA_ARGS__); \ > - break; \ > - case ODP_LOG_PRINT: \ > - odp_override_log(level, " " fmt, ##__VA_ARGS__); \ > - break; \ > - case ODP_LOG_ABORT: \ > - odp_override_log(level, "%s:%d:%s(): " fmt, __FILE__, \ > - __LINE__, __func__, ##__VA_ARGS__); \ > - abort(); \ > - break; \ > - case ODP_LOG_UNIMPLEMENTED: \ > - odp_override_log(level, \ > - "%s:%d:The function %s() is not implemented\n" \ > - fmt, __FILE__, __LINE__, __func__, ##__VA_ARGS__); \ > - break; \ > - default: \ > - odp_override_log(level, "Unknown LOG level"); \ > - break;\ > - } \ > -} while (0) > + odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \ > + __LINE__, __func__, ##__VA_ARGS__) > > /** > * 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__) > + odp_override_log(ODP_LOG_PRINT, " " fmt, ##__VA_ARGS__) > > /** > * Log debug message if DEBUG flag is set. > */ > #define ODP_DBG(fmt, ...) \ > - ODP_LOG(ODP_LOG_DBG, fmt, ##__VA_ARGS__) > + do { \ > + if (ODP_DEBUG_PRINT == 1) \ > + ODP_LOG(ODP_LOG_DBG, fmt, ##__VA_ARGS__);\ > + } while (0) > > /** > * Log error message. > @@ -153,7 +129,10 @@ do { \ > * This function should not return. > */ > #define ODP_ABORT(fmt, ...) \ > - ODP_LOG(ODP_LOG_ABORT, fmt, ##__VA_ARGS__) > + do { \ > + ODP_LOG(ODP_LOG_ABORT, fmt, ##__VA_ARGS__); \ > + abort(); \ > + } while (0) > > /** > * @} > diff --git a/platform/linux-generic/include/odp_debug_internal.h b/platform/linux-generic/include/odp_debug_internal.h > index a87552f..ee3c543 100644 > --- a/platform/linux-generic/include/odp_debug_internal.h > +++ b/platform/linux-generic/include/odp_debug_internal.h > @@ -25,8 +25,10 @@ extern "C" { > /** > * This macro is used to indicate when a given function is not implemented > */ > -#define ODP_UNIMPLEMENTED(fmt, ...) \ > - ODP_LOG(ODP_LOG_UNIMPLEMENTED, fmt, ##__VA_ARGS__) > +#define ODP_UNIMPLEMENTED() \ > + odp_override_log(ODP_LOG_UNIMPLEMENTED, \ > + "%s:%d:The function %s() is not implemented\n", \ > + __FILE__, __LINE__, __func__) > > #ifdef __cplusplus > } >
On 12/02/2014 06:42 PM, Zoltan Kiss wrote: > Hi, > > At the moment, the careless developer look at "enum odp_log_level" and > starts using ODP_LOG(ODP_LOG_DBG, ...) instead of ODP_DBG. > I think it would be good to provide either ODP_LOG(loglevel, ...) or > ODP_[loglevel](...), but not both. These are internal functions, so it is up to a platform developer/maintainer to decide how he want to use them.
On 02/12/14 17:07, Taras Kondratiuk wrote: > On 12/02/2014 06:42 PM, Zoltan Kiss wrote: >> Hi, >> >> At the moment, the careless developer look at "enum odp_log_level" and >> starts using ODP_LOG(ODP_LOG_DBG, ...) instead of ODP_DBG. >> I think it would be good to provide either ODP_LOG(loglevel, ...) or >> ODP_[loglevel](...), but not both. > > These are internal functions, so it is up to a platform > developer/maintainer to decide how he want to use them. platform/linux-generic/include/api/odp_debug.h can be used by applications, as my recent patch showed, the OVS interface does that for example. And this problem applies to ODP_LOG_ABORT as well: ODP_ABORT will call abort(), while ODP_LOG(ODP_LOG_ABORT, ...) doesn't. I think you can just drop the wholw ODP_LOG macro completely, as far as I see nothing uses it anymore. Zoli
On 12/02/2014 07:50 PM, Zoltan Kiss wrote: > > > On 02/12/14 17:07, Taras Kondratiuk wrote: >> On 12/02/2014 06:42 PM, Zoltan Kiss wrote: >>> Hi, >>> >>> At the moment, the careless developer look at "enum odp_log_level" and >>> starts using ODP_LOG(ODP_LOG_DBG, ...) instead of ODP_DBG. >>> I think it would be good to provide either ODP_LOG(loglevel, ...) or >>> ODP_[loglevel](...), but not both. >> >> These are internal functions, so it is up to a platform >> developer/maintainer to decide how he want to use them. > > platform/linux-generic/include/api/odp_debug.h can be used by > applications, as my recent patch showed, the OVS interface does that for > example. That is a mistake. We agreed that these functions are not public and can be used only inside of platform implementation. We should move them to odp_debug_internal.h to avoid confusions. The only thing that should stay in odp_debug.h it a declaration of odp_override_log(). > And this problem applies to ODP_LOG_ABORT as well: ODP_ABORT > will call abort(), while ODP_LOG(ODP_LOG_ABORT, ...) doesn't. > I think you can just drop the wholw ODP_LOG macro completely, as far as > I see nothing uses it anymore. ODP_LOG is used as helper macro. If you wish I can call it _ODP_LOG, but honestly I don't see a real reason to do it.
On 03/12/14 08:41, Taras Kondratiuk wrote: > On 12/02/2014 07:50 PM, Zoltan Kiss wrote: >> >> >> On 02/12/14 17:07, Taras Kondratiuk wrote: >>> On 12/02/2014 06:42 PM, Zoltan Kiss wrote: >>>> Hi, >>>> >>>> At the moment, the careless developer look at "enum odp_log_level" and >>>> starts using ODP_LOG(ODP_LOG_DBG, ...) instead of ODP_DBG. >>>> I think it would be good to provide either ODP_LOG(loglevel, ...) or >>>> ODP_[loglevel](...), but not both. >>> >>> These are internal functions, so it is up to a platform >>> developer/maintainer to decide how he want to use them. >> >> platform/linux-generic/include/api/odp_debug.h can be used by >> applications, as my recent patch showed, the OVS interface does that for >> example. > > That is a mistake. We agreed that these functions are not public and can > be used only inside of platform implementation. > We should move them to odp_debug_internal.h to avoid confusions. > The only thing that should stay in odp_debug.h it a declaration of > odp_override_log(). Yeah, I agree with the moving, it shouldn't be accessible to applications. > >> And this problem applies to ODP_LOG_ABORT as well: ODP_ABORT >> will call abort(), while ODP_LOG(ODP_LOG_ABORT, ...) doesn't. >> I think you can just drop the wholw ODP_LOG macro completely, as far as >> I see nothing uses it anymore. > > ODP_LOG is used as helper macro. If you wish I can call it _ODP_LOG, but > honestly I don't see a real reason to do it. I mean ODP_LOG is used now twice, in ODP_DBG and ODP_ABORT, to add (__FILE__, __LINE__, __func__) to the string. In the meantime ODP_PRINT and ODP_UNIMPLEMENTED calls odp_override_log directly. I think we should just ditch ODP_LOG and call odp_override_log directly from everywhere, so noone gets tempted to use it in a platform implementation. Zoli
On 12/03/2014 04:15 PM, Zoltan Kiss wrote: > > > On 03/12/14 08:41, Taras Kondratiuk wrote: >> On 12/02/2014 07:50 PM, Zoltan Kiss wrote: >>> And this problem applies to ODP_LOG_ABORT as well: ODP_ABORT >>> will call abort(), while ODP_LOG(ODP_LOG_ABORT, ...) doesn't. >>> I think you can just drop the wholw ODP_LOG macro completely, as far as >>> I see nothing uses it anymore. >> >> ODP_LOG is used as helper macro. If you wish I can call it _ODP_LOG, but >> honestly I don't see a real reason to do it. > I mean ODP_LOG is used now twice, in ODP_DBG and ODP_ABORT, to add > (__FILE__, __LINE__, __func__) to the string. In the meantime ODP_PRINT > and ODP_UNIMPLEMENTED calls odp_override_log directly. I think we should > just ditch ODP_LOG and call odp_override_log directly from everywhere, > so noone gets tempted to use it in a platform implementation. ODP_LOG is actually used three times (ODP_ERR is not shown in the patch). ODP_PRINT was also using ODP_LOG until recent changes. IMO even with three places it worth to have a common ODP_LOG instead of copying format string.
On 12/03/2014 05:15 PM, Zoltan Kiss wrote:
> ODP_DBG and ODP_ABORT, to add (__FILE__, __LINE__, __func__)
Is there any reason to print FILE? If we know function name and line we
can find that place. Printing file just makes output longer.
I like format like that:
"%s:%d:", func, line
Maxim.
On 12/03/2014 05:08 PM, Maxim Uvarov wrote: > On 12/03/2014 05:15 PM, Zoltan Kiss wrote: >> ODP_DBG and ODP_ABORT, to add (__FILE__, __LINE__, __func__) > Is there any reason to print FILE? If we know function name and line we > can find that place. Printing file just makes output longer. > > I like format like that: > "%s:%d:", func, line This patch doesn't aim to change the output format. It can be changed in a separate patch if needed.
On 12/03/2014 06:12 PM, Taras Kondratiuk wrote: > On 12/03/2014 05:08 PM, Maxim Uvarov wrote: >> On 12/03/2014 05:15 PM, Zoltan Kiss wrote: >>> ODP_DBG and ODP_ABORT, to add (__FILE__, __LINE__, __func__) >> Is there any reason to print FILE? If we know function name and line we >> can find that place. Printing file just makes output longer. >> >> I like format like that: >> "%s:%d:", func, line > > This patch doesn't aim to change the output format. > It can be changed in a separate patch if needed. Yes, sure it's should go separately. Maxim.
diff --git a/platform/linux-generic/include/api/odp_debug.h b/platform/linux-generic/include/api/odp_debug.h index 4b51038..13c3939 100644 --- a/platform/linux-generic/include/api/odp_debug.h +++ b/platform/linux-generic/include/api/odp_debug.h @@ -99,48 +99,24 @@ 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: \ - odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \ - __LINE__, __func__, ##__VA_ARGS__); \ - break; \ - case ODP_LOG_DBG: \ - if (ODP_DEBUG_PRINT == 1) \ - odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \ - __LINE__, __func__, ##__VA_ARGS__); \ - break; \ - case ODP_LOG_PRINT: \ - odp_override_log(level, " " fmt, ##__VA_ARGS__); \ - break; \ - case ODP_LOG_ABORT: \ - odp_override_log(level, "%s:%d:%s(): " fmt, __FILE__, \ - __LINE__, __func__, ##__VA_ARGS__); \ - abort(); \ - break; \ - case ODP_LOG_UNIMPLEMENTED: \ - odp_override_log(level, \ - "%s:%d:The function %s() is not implemented\n" \ - fmt, __FILE__, __LINE__, __func__, ##__VA_ARGS__); \ - break; \ - default: \ - odp_override_log(level, "Unknown LOG level"); \ - break;\ - } \ -} while (0) + odp_override_log(level, "%s:%d:%s():" fmt, __FILE__, \ + __LINE__, __func__, ##__VA_ARGS__) /** * 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__) + odp_override_log(ODP_LOG_PRINT, " " fmt, ##__VA_ARGS__) /** * Log debug message if DEBUG flag is set. */ #define ODP_DBG(fmt, ...) \ - ODP_LOG(ODP_LOG_DBG, fmt, ##__VA_ARGS__) + do { \ + if (ODP_DEBUG_PRINT == 1) \ + ODP_LOG(ODP_LOG_DBG, fmt, ##__VA_ARGS__);\ + } while (0) /** * Log error message. @@ -153,7 +129,10 @@ do { \ * This function should not return. */ #define ODP_ABORT(fmt, ...) \ - ODP_LOG(ODP_LOG_ABORT, fmt, ##__VA_ARGS__) + do { \ + ODP_LOG(ODP_LOG_ABORT, fmt, ##__VA_ARGS__); \ + abort(); \ + } while (0) /** * @} diff --git a/platform/linux-generic/include/odp_debug_internal.h b/platform/linux-generic/include/odp_debug_internal.h index a87552f..ee3c543 100644 --- a/platform/linux-generic/include/odp_debug_internal.h +++ b/platform/linux-generic/include/odp_debug_internal.h @@ -25,8 +25,10 @@ extern "C" { /** * This macro is used to indicate when a given function is not implemented */ -#define ODP_UNIMPLEMENTED(fmt, ...) \ - ODP_LOG(ODP_LOG_UNIMPLEMENTED, fmt, ##__VA_ARGS__) +#define ODP_UNIMPLEMENTED() \ + odp_override_log(ODP_LOG_UNIMPLEMENTED, \ + "%s:%d:The function %s() is not implemented\n", \ + __FILE__, __LINE__, __func__) #ifdef __cplusplus }
Move additional functionality out of ODP_LOG. Keep only logging. Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> --- platform/linux-generic/include/api/odp_debug.h | 43 +++++--------------- .../linux-generic/include/odp_debug_internal.h | 6 ++- 2 files changed, 15 insertions(+), 34 deletions(-)