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 [off-list ref] wrote:

On Thu, 18 Feb 2021 14:12:53 +0800, Li Feng wrote:

quoted

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

quoted

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

quoted

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

quoted

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

[...]

quoted

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

quoted

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

[...]

quoted

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