Re: [dpdk-dev] [PATCH v1 1/3] net/hyperv: introduce MS Hyper-V platform driver

[Thomas Monjalon <thomas@monjalon.net>]

19/12/2017 11:01, Adrien Mazarguil:
> On Mon, Dec 18, 2017 at 01:17:51PM -0800, Stephen Hemminger wrote:
> > On Mon, 18 Dec 2017 20:54:16 +0100
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> > 
> > > > > +#endif /* RTE_LIBRTE_HYPERV_DEBUG */
> > > > > +
> > > > > +#define DEBUG(...) PMD_DRV_LOG(DEBUG, __VA_ARGS__)
> > > > > +#define INFO(...) PMD_DRV_LOG(INFO, __VA_ARGS__)
> > > > > +#define WARN(...) PMD_DRV_LOG(WARNING, __VA_ARGS__)
> > > > > +#define ERROR(...) PMD_DRV_LOG(ERR, __VA_ARGS__)
> > > > > +  
> > > > 
> > > > Please don't use DEBUG() etc macros. It makes it easier for tools that do
> > > > global updates or scans if all drivers use the same model of PMD_DRV_LOG  
> > > 
> > > The new standard is to use dynamic logtype.
> > 
> > Agree, please use dynamic logging, and also don't redefine new macros like DEBUG/INFO/WARN/ERROR.
> > Instead use PMD_DRV_LOG or equivalent macros.
> 
> Wait, the above definitions are only convenience wrappers to PMD_DRV_LOG(),
> itself a wrapper to RTE_LOG(), itself a wrapper to rte_log(), their presence
> is not triggered according to compilation options, did I miss something?
> 
> Let me bring back some context from the original patch:
> 
>  #ifdef RTE_LIBRTE_HYPERV_DEBUG
> 
>  #define PMD_DRV_LOG(level, ...) \
>  	RTE_LOG(level, PMD, \
>  		RTE_FMT("%s:%u: %s(): " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
>  			strrchr("/" __FILE__, '/') + 1, \
>  			__LINE__, \
>  			__func__, \
>  			RTE_FMT_TAIL(__VA_ARGS__,)))
> 
>  #else /* RTE_LIBRTE_HYPERV_DEBUG */
> 
>  #define PMD_DRV_LOG(level, ...) \
>  	RTE_LOG(level, PMD, \
>  		RTE_FMT(RTE_STR(HYPERV_DRIVER) ": " \
>  			RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
>  		RTE_FMT_TAIL(__VA_ARGS__,)))
> 
>  #endif /* RTE_LIBRTE_HYPERV_DEBUG */
> 
>  #define DEBUG(...) PMD_DRV_LOG(DEBUG, __VA_ARGS__)
>  #define INFO(...) PMD_DRV_LOG(INFO, __VA_ARGS__)
>  #define WARN(...) PMD_DRV_LOG(WARNING, __VA_ARGS__)
>  #define ERROR(...) PMD_DRV_LOG(ERR, __VA_ARGS__)
> 
> Enabling RTE_LIBRTE_HYPERV_DEBUG adds file and line information to log
> output, messages are otherwise unaffected by that compilation option. Adding
> this information required some sort of wrapper to avoid needless clutter.
> 
> Nothing against outputting file/line information when compiled in debug mode
> right?

I am not sure __FILE__, __LINE__ and __func__ are so much useful.
The log message should be unique enough.

> > The base rule here is that all drivers should look the same as much
> > as reasonably possible. This makes reviewers of other subsystems more likely
> > to see problems. It also allows for later changes where some developer does a global
> > improvement across many PMD's.
> > 
> > Drivers should not be snowflakes, each one is not unique.
> 
> Point taken, do you confirm replacing i.e. WARN(...) with
> PMD_DRV_LOG(WARN, ...) and friends is all that's needed?

You need to remove the compile-time option for DEBUG,
and rely on dynamic log type, thanks to rte_log_register().