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

[Feng Li <lifeng1519@gmail.com>]

Hi Dmitry,
Thanks for your comments. I will submit the next patch to address your concern.

On Fri, Feb 19, 2021 at 4:11 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> On Thu, 18 Feb 2021 14:12:53 +0800, Li Feng wrote:
> > By default, 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 is another choice for this scenario.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v4: Fix the code style.
> > v3: Rename the func, change the comments, add funcs in version.map.
> > v2: Simplify the code.
>
> While I support the feature in general, its current design is imperfect both
> at interface level (documentation, corner cases) and at interaction with the
> rest of EAL logging subsystem (hijacking Linux EAL internals). IMO, proper
> implementation would rework rte_vlog() to print to buffer, then pass it to
> current logging function. But rte_openlog_stream() has to be supported still.
> What do others think?
OK, I have reworked on rte_vlog, it looks more reasonable.
>
> > +/**
> > + * Define a logging write function.
> > + */
> > +typedef ssize_t rte_log_write_function(void *cookie, const char *buf,
> > +     size_t size);
>
> You're supposed to document callback parameters and behavior here.
> What about its thread-safety?
> It's not obvious what "cookie" means (suggesting "context").
Acked.
>
> > +
> > +/**
> > + * Change the default stream's write action that will be used by the logging
> > + * system.
> > + *
> > + * This should be done before the 'rte_eal_init' call. And the
> > + * 'rte_openlog_stream' call will override this action.
> > + *
> > + * @param logf
> > + *   Pointer to the log write function.
> > + */
> > +__rte_experimental
> > +void
> > +rte_log_sink_set(rte_log_write_function *logf);
>
> Since this API is currently Linux-only, it must report lack of support for
> FreeBSD and Windows, presumably via return value.
Acked.

>
> > +
> > +/**
> > + * Retrieve the log function used by the logging system (see rte_log_sink_set()
> > + * to change it).
> > + *
> > + * @return
> > + *   Pointer to the log function.
> > + */
> > +__rte_experimental
> > +rte_log_write_function*
> > +rte_log_sink_get(void);
>
> Doesn't checkpatch complain about "ret_type*" vs "ret_type *"?
Actually, checkpatch treats it ok.

>
> [...]
> > +/**
> > + * Change the default stream's write action that will be used by the logging
> > + * system.
> > + *
> > + * This should be done before the 'rte_eal_init' call. And the
> > + * 'rte_openlog_stream' call will override this action.
> > + */
>
> It's not very helpful to repeat public documnetation.
> Here is a good place to explain implementation, if required.
Acked.

>
> > +void
> > +rte_log_sink_set(rte_log_write_function *logf)
> > +{
> > +     console_log_func.write = logf;
> > +}
>
> 1. What if NULL is passed? An app will likely crash far from the culprit.
> 2. This breaks EAL writing to syslog. It's probably intended, but please at
> least document such behavior.

OK, I think that if the logf is NULL, then keep the original function.

>
> [...]
> > +/*
> > + * Retrieve the default log write function.
> > + */
> > +rte_log_write_function*
> > +rte_log_sink_get(void)
> > +{
> > +     return NULL;
> > +}
>
> I'd say it's an API inconsistency that you can write logs but sink is not
> callable.
I have rewritten this patch, it will be safe.