[v4] log: support custom log function
Checks
Commit Message
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
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.
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.
@@ -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.
*
@@ -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;
+}
@@ -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 {
@@ -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;
+}