Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-10-11 09:30, John Ousterhout:
> On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> >
> > 2016-10-10 15:39, John Ousterhout:
> > > ...
> > >
> > > Note: I see from the code that Linux and BSD set different default
> streams:
> > > Linux uses stdout, while BSD uses stderr. This patch retains the
> distinction,
> > > though I'm not sure why it is there.
> >
> > I don't know either.
> > What is best between stdout and stderr for logs?
> 
> I would guess that stdout makes more sense, since most log entries describe
> normal operation, not errors. I'm happy to make these consistent, but this
> would introduce a behavior change for BSD (which currently uses stderr);
> would that be considered antisocial?

No, that's OK to use stdout on BSD.

> > > -int
> > > -rte_eal_common_log_init(FILE *default_log)
> > > +void
> > > +rte_eal_log_set_default(FILE *default_log)
> > >  {
> > >       default_log_stream = default_log;
> > > -     rte_openlog_stream(default_log);
> > >
> > >  #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
> > >       RTE_LOG(NOTICE, EAL, "Debug logs available - lower
> performance\n");
> > >  #endif
> > > -
> > > -     return 0;
> > >  }
> >
> > Do we really need a function for that?
> > Why not just initialize default_log_stream statically?
> 
> Right now, different platforms have different defaults. BSD uses stderr
> always. Linux starts out with stdout as the default, but later during
> initialization it switches to a different default that logs messages both
> to  standard output and also to syslog. I don't have enough experience with
> DPDK to know whether a single approach is really right for all systems (or
> which approach it should be), and since I'm a DPDK newbie I thought it best
> to take a more conservative approach and avoid behavioral changes. My
> personal preference would be to minimize mission creep with this patch and
> leave that behavior in place for someone with more expertise to deal with
> later (and this issue is orthogonal to the main goal of the patch). But, if
> unifying the log defaults is considered essential to the patch (and is
> noncontroversial), I'm willing to implement it.

OK sorry, I'm mixing things.

1/ When removing early log functions, you are replacing early init with
a default set to stderr/stdout via rte_eal_log_set_default.
I think you can just set statically to stdout:
	static FILE *default_log_stream = stdout;

2/ Yes, on Linux, a more complex stream with stdout + syslog is set.
It is OK to use rte_eal_log_set_default for that usage.
Note that there is a stream which is not used and can be removed in eal_private.h:
	extern FILE *eal_default_log_stream;
Other note: rte_eal_log_set_default is not a public function so should be
named eal_log_set_default.

3/ When calling rte_eal_log_set_default on BSD from rte_eal_log_init,
you can keep stderr but an empty function would be better because
it is not called and already set (to stderr or stdout if 1/).

4/ rte_eal_log_init can be called earlier to replace early init.

5/ It would be simpler to understand by splitting in two patches
(remove early log + use non default log)

I understand that you prefer to focus on your fix and I'm more or less
suggesting a cleanup of logging. That's why I can do the first cleanup
patch if you are really not confortable with it. (I feel you could do it)
Just let me know.