Re: [dpdk-dev] [PATCH v2] log: support custom log function

[Feng Li <lifeng1519@gmail.com>]

On Tue, Feb 9, 2021 at 6:40 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> On Mon, 8 Feb 2021 14:58:29 +0800, Feng Li wrote:
> > > > +/**
> > > > + * Usage function typedef used by the application usage function.
> > >
> > > It's unrelated to the following typedef purpose, is it?
> > It's borrowed from the front typedef sentence.
>
> Doesn't make much sense here anyway.
Acked.

>
> > > > + *
> > > > + * Use this function typedef to define a logger formatter.
> > > > + */
> > > > +typedef cookie_write_function_t rte_log_func_t;
> > >
> > > "cookie_write_function_t" is not standard C, please write this type
> > > explicitly. POSIX reserves "_t" suffix, "rte_" prefix is enough.
> > Fix to this?
> > typedef cookie_write_function_t rte_cookie_write_function;
>
> You cannot expose "cookie_write_function_t" in public API, because it is not
> portable. You have to write out the type in full.
> I'd replace "cookie" with "log".
Acked.

>
> > void rte_log_sink_set(rte_cookie_write_function* cookie_write);
> > rte_cookie_write_function* rte_log_sink_get();
> > Right?
>
> OK.
>
> > > > +void
> > > > +rte_eal_set_log_func(rte_log_func_t *logf)
> > > > +{
> > > > +    console_log_func.write = logf;
> > > > +}
>
> Is this correct? AFAIK, updating the member has no effect, unless you call
> fopencookie() with an updated structure.
The call order is:
rte_eal_set_log_func(logf);
...
rte_eal_init();

Because the rte_eal_init will call rte_eal_log_init, so set the struct
before rte_eal_init.
And the logs in `rte_eal_init` will also be redirected.

>
> Your new callbacks don't seem to be integrated with DPDK logging system for
> all platforms ("eal_common_log.c", "windows/eal_log.c"). If the new API and
> rte_openlog_stream() cancel effect of each other, this should be documented.
>

Looks like `rte_eal_log_init` is defined in Linux and Windows, and
Windows only redirects
the log to stderr. So Windows doesn't support this feature.

rte_openlog_stream has a higher priority than rte_eal_set_log_func.
If it isn't called, then 'rte_eal_set_log_func' call will work.
I will update the comments of this API.

Thanks.