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

[John Ousterhout <ouster@cs.stanford.edu>]

All of your suggestions look reasonable and fairly straightforward; I'll
work on a new patch that includes them.

Given that rte_eal_log_init is a no-op (and won't even be invoked), would
it be better to remove that function completely, and even delete the file
containing it (eal_log.c), or is it better to retain the empty function in
order to maintain a parallel structure with Linux? Personally I'd lean
towards deleting the file. As it stands, the interface to that function
doesn't even make sense for BSD; the arguments were chosen for Linux and
are ignored in BSD.

Let me know your preference.

-John-

On Tue, Oct 11, 2016 at 1:30 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

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