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 <tho...@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().
