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

[Feng Li <lifeng1519@gmail.com>]

Thanks for your comments.

Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> 于2021年2月6日周六 上午3:32写道：
>
> On Sat,  6 Feb 2021 01:42:04 +0800, Li Feng wrote:
> > Currently, the dpdk log is out to stdout/stderr and syslog.
> > The rte_openlog_stream could set an external FILE* stream, but it asks the
> > consumer to give it a FILE* pointer.
> > For C++ or other languages, it's hard to get a libc FILE*.
> >
> > Support to set a hook method is another choice for this scenario.
>
> It's "DPDK"; and "hard" sounds vague. I'd state the real issue such that
> API to change DPDK log sink mandates the (direct) use of libc in consumer
> application. To invoke arbitrary function, consumers have to use facilities
> that are non-standard (fopencookie) or OS-specific (pipes).
>
> >
> > Change-Id: I3b2419cc2fe5256205daa8077fd8862a8e58fb6c
>
> What is this?
>
It's Gerrit's Change-ID, I will remove it.

> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v2: simplify the code.
> >
> >  lib/librte_eal/include/rte_eal.h | 16 ++++++++++++++++
> >  lib/librte_eal/linux/eal_log.c   | 10 ++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> > index eaf6469e5..bd6cf554b 100644
> > --- a/lib/librte_eal/include/rte_eal.h
> > +++ b/lib/librte_eal/include/rte_eal.h
> > @@ -501,6 +501,22 @@ rte_eal_mbuf_user_pool_ops(void);
> >  const char *
> >  rte_eal_get_runtime_dir(void);
> >
> > +/**
> > + * 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.

>
> > + *
> > + * 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;

>
> > +
> > +/**
> > + * Set a customized logger.
> > + *
> > + * @param logf
> > + *   The customized logger function.
> > + */
> > +void
> > +rte_eal_set_log_func(rte_log_func_t *logf);
> > +
>
> How about rte_log_sink_set() and a companion rte_log_sink_get() or returning
> the previous value when setting a new one? Also, like in fopencookie(), a
> cookie argument is needed.

void rte_log_sink_set(rte_cookie_write_function* cookie_write);
rte_cookie_write_function* rte_log_sink_get();
Right?

>
> New functions must be marker __rte_experimental and added to .map and .def
> files. Please use devtools/checkpatches.sh that will run checks like that for
> your commits.
>
I have run the `devtools/checkpatches.sh`, it reports valid, so weird.
I'll fix it.


> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
> > index 43c8460bf..c0a7a12ab 100644
> > --- a/lib/librte_eal/linux/eal_log.c
> > +++ b/lib/librte_eal/linux/eal_log.c
> > @@ -60,3 +60,13 @@ rte_eal_log_init(const char *id, int facility)
> >
> >       return 0;
> >  }
> > +
> > +/*
> > + * Set the customized logger, it will override the default action, which is
> > + * writing to syslog and stdout.
> > + */
> > +void
> > +rte_eal_set_log_func(rte_log_func_t *logf)
> > +{
> > +    console_log_func.write = logf;
> > +}
>