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

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

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?

> +/**
> + * 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").

> +
> +/**
> + * 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.

> +
> +/**
> + * 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 *"?

[...]
> +/**
> + * 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.

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

[...]
> +/*
> + * 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.