diff mbox series

[v4] log: support custom log function

Message ID 20210218061253.2812991-1-fengli@smartx.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers show
Series [v4] log: support custom log function | expand

Checks

Context Check Description
ci/iol-testing success Testing PASS
ci/github-robot success github build: passed
ci/travis-robot fail travis build: failed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Li Feng Feb. 18, 2021, 6:12 a.m. UTC
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.

 lib/librte_eal/include/rte_log.h | 31 +++++++++++++++++++++++++++++++
 lib/librte_eal/linux/eal_log.c   | 23 +++++++++++++++++++++++
 lib/librte_eal/version.map       |  2 ++
 lib/librte_eal/windows/eal_log.c | 19 +++++++++++++++++++
 4 files changed, 75 insertions(+)

Comments

Dmitry Kozlyuk Feb. 19, 2021, 8:11 a.m. UTC | #1
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.
Feng Li Feb. 23, 2021, 11:22 a.m. UTC | #2
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.
diff mbox series

Patch

diff --git a/lib/librte_eal/include/rte_log.h b/lib/librte_eal/include/rte_log.h
index 173004fd7..adf299610 100644
--- a/lib/librte_eal/include/rte_log.h
+++ b/lib/librte_eal/include/rte_log.h
@@ -97,6 +97,37 @@  int rte_openlog_stream(FILE *f);
  */
 FILE *rte_log_get_stream(void);
 
+/**
+ * Define a logging write function.
+ */
+typedef ssize_t rte_log_write_function(void *cookie, const char *buf,
+	size_t size);
+
+/**
+ * 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);
+
+/**
+ * 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);
+
 /**
  * Set the global log level.
  *
diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
index 43c8460bf..fb3ac3f14 100644
--- a/lib/librte_eal/linux/eal_log.c
+++ b/lib/librte_eal/linux/eal_log.c
@@ -60,3 +60,26 @@  rte_eal_log_init(const char *id, int facility)
 
 	return 0;
 }
+
+/**
+ * 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.
+ */
+void
+rte_log_sink_set(rte_log_write_function *logf)
+{
+	console_log_func.write = logf;
+}
+
+/**
+ * Retrieve the log function used by the logging system (see rte_log_sink_set()
+ * to change it).
+ */
+rte_log_write_function*
+rte_log_sink_get(void)
+{
+	return console_log_func.write;
+}
diff --git a/lib/librte_eal/version.map b/lib/librte_eal/version.map
index fce90a112..04d651912 100644
--- a/lib/librte_eal/version.map
+++ b/lib/librte_eal/version.map
@@ -412,6 +412,8 @@  EXPERIMENTAL {
 	rte_thread_tls_key_delete;
 	rte_thread_tls_value_get;
 	rte_thread_tls_value_set;
+	rte_log_sink_set;
+	rte_log_sink_get;
 };
 
 INTERNAL {
diff --git a/lib/librte_eal/windows/eal_log.c b/lib/librte_eal/windows/eal_log.c
index 875981f13..589b47f27 100644
--- a/lib/librte_eal/windows/eal_log.c
+++ b/lib/librte_eal/windows/eal_log.c
@@ -14,3 +14,22 @@  rte_eal_log_init(__rte_unused const char *id, __rte_unused int facility)
 
 	return 0;
 }
+
+/*
+ * Set the customized logger, it will override the default stream write action,
+ * which is writing to syslog and stdout.
+ */
+void
+rte_log_sink_set(rte_log_write_function *logf)
+{
+	RTE_SET_USED(logf);
+}
+
+/*
+ * Retrieve the default log write function.
+ */
+rte_log_write_function*
+rte_log_sink_get(void)
+{
+	return NULL;
+}