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

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

[Li Feng]

Currently, the dpdk log is out to stdout/stderr and syslog.
We should support to output the log to another please, e.g. file or
glog.

Signed-off-by: Li Feng <fengli@smartx.com>
---
 lib/librte_eal/common/eal_private.h | 10 ----------
 lib/librte_eal/include/rte_eal.h    | 22 ++++++++++++++++++++++
 lib/librte_eal/linux/eal.c          |  2 +-
 lib/librte_eal/linux/eal_log.c      | 13 ++++++++-----
 lib/librte_eal/windows/eal.c        |  2 +-
 lib/librte_eal/windows/eal_log.c    |  2 +-
 6 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 4684c4c7d..8cfb399b8 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -143,16 +143,6 @@ int rte_eal_memory_init(void);
  */
 int rte_eal_timer_init(void);
 
-/**
- * Init the default log stream
- *
- * This function is private to EAL.
- *
- * @return
- *   0 on success, negative on error
- */
-int rte_eal_log_init(const char *id, int facility);
-
 /**
  * Save the log regexp for later
  */
diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
index eaf6469e5..f07d89f5f 100644
--- a/lib/librte_eal/include/rte_eal.h
+++ b/lib/librte_eal/include/rte_eal.h
@@ -114,6 +114,28 @@ int rte_eal_iopl_init(void);
  */
 int rte_eal_init(int argc, char **argv);
 
+/**
+ * Usage function typedef used by the application usage function.
+ *
+ * Use this function typedef to define a logger formatter.
+ */
+typedef cookie_write_function_t rte_log_func_t;
+
+/**
+ * Init the default log stream
+ *
+ * @param id
+ *   The openlog's first argument.
+ * @param facility
+ *   The openlog's third argument.
+ * @param logf
+ *   The customized logger function, if it's set, the id and facility will
+ *   be ignored.
+ * @return
+ *   0 on success, negative on error
+ */
+int rte_eal_log_init(const char *id, int facility, rte_log_func_t *logf);
+
 /**
  * Clean up the Environment Abstraction Layer (EAL)
  *
diff --git a/lib/librte_eal/linux/eal.c b/lib/librte_eal/linux/eal.c
index 32b48c3de..62747b3a7 100644
--- a/lib/librte_eal/linux/eal.c
+++ b/lib/librte_eal/linux/eal.c
@@ -1160,7 +1160,7 @@ rte_eal_init(int argc, char **argv)
 #endif
 	}
 
-	if (rte_eal_log_init(logid, internal_conf->syslog_facility) < 0) {
+	if (rte_eal_log_init(logid, internal_conf->syslog_facility, NULL) < 0) {
 		rte_eal_init_alert("Cannot init logging.");
 		rte_errno = ENOMEM;
 		__atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
index 43c8460bf..f985b3d36 100644
--- a/lib/librte_eal/linux/eal_log.c
+++ b/lib/librte_eal/linux/eal_log.c
@@ -37,18 +37,21 @@ console_log_write(__rte_unused void *c, const char *buf, size_t size)
 	return ret;
 }
 
-static cookie_io_functions_t console_log_func = {
-	.write = console_log_write,
-};
-
 /*
  * set the log to default function, called during eal init process,
  * once memzones are available.
  */
 int
-rte_eal_log_init(const char *id, int facility)
+rte_eal_log_init(const char *id, int facility, rte_log_func_t *logf)
 {
 	FILE *log_stream;
+	cookie_io_functions_t console_log_func;
+
+	if (logf) {
+		console_log_func.write = logf;
+	} else {
+		console_log_func.write = console_log_write;
+	}
 
 	log_stream = fopencookie(NULL, "w+", console_log_func);
 	if (log_stream == NULL)
diff --git a/lib/librte_eal/windows/eal.c b/lib/librte_eal/windows/eal.c
index 1e5f6576f..74fdd076a 100644
--- a/lib/librte_eal/windows/eal.c
+++ b/lib/librte_eal/windows/eal.c
@@ -266,7 +266,7 @@ rte_eal_init(int argc, char **argv)
 		eal_get_internal_configuration();
 	int ret;
 
-	rte_eal_log_init(NULL, 0);
+	rte_eal_log_init(NULL, 0, NULL);
 
 	eal_log_level_parse(argc, argv);
 
diff --git a/lib/librte_eal/windows/eal_log.c b/lib/librte_eal/windows/eal_log.c
index 875981f13..31f556908 100644
--- a/lib/librte_eal/windows/eal_log.c
+++ b/lib/librte_eal/windows/eal_log.c
@@ -6,7 +6,7 @@
 
 /* set the log to default function, called during eal init process. */
 int
-rte_eal_log_init(__rte_unused const char *id, __rte_unused int facility)
+rte_eal_log_init(__rte_unused const char *id, __rte_unused int facility, rte_log_func_t *logf)
 {
 	rte_openlog_stream(stderr);
 
-- 
2.29.2

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

[Feng Li]

Li Feng <fengli@smartx.com> 于2021年2月5日周五 下午7:24写道：
>
> Currently, the dpdk log is out to stdout/stderr and syslog.
> We should support to output the log to another please, e.g. file or
> glog.
Sorry, sed 's/please/place/g'.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
>  lib/librte_eal/common/eal_private.h | 10 ----------
>  lib/librte_eal/include/rte_eal.h    | 22 ++++++++++++++++++++++
>  lib/librte_eal/linux/eal.c          |  2 +-
>  lib/librte_eal/linux/eal_log.c      | 13 ++++++++-----
>  lib/librte_eal/windows/eal.c        |  2 +-
>  lib/librte_eal/windows/eal_log.c    |  2 +-
>  6 files changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 4684c4c7d..8cfb399b8 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -143,16 +143,6 @@ int rte_eal_memory_init(void);
>   */
>  int rte_eal_timer_init(void);
>
> -/**
> - * Init the default log stream
> - *
> - * This function is private to EAL.
> - *
> - * @return
> - *   0 on success, negative on error
> - */
> -int rte_eal_log_init(const char *id, int facility);
> -
>  /**
>   * Save the log regexp for later
>   */
> diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> index eaf6469e5..f07d89f5f 100644
> --- a/lib/librte_eal/include/rte_eal.h
> +++ b/lib/librte_eal/include/rte_eal.h
> @@ -114,6 +114,28 @@ int rte_eal_iopl_init(void);
>   */
>  int rte_eal_init(int argc, char **argv);
>
> +/**
> + * Usage function typedef used by the application usage function.
> + *
> + * Use this function typedef to define a logger formatter.
> + */
> +typedef cookie_write_function_t rte_log_func_t;
> +
> +/**
> + * Init the default log stream
> + *
> + * @param id
> + *   The openlog's first argument.
> + * @param facility
> + *   The openlog's third argument.
> + * @param logf
> + *   The customized logger function, if it's set, the id and facility will
> + *   be ignored.
> + * @return
> + *   0 on success, negative on error
> + */
> +int rte_eal_log_init(const char *id, int facility, rte_log_func_t *logf);
> +
>  /**
>   * Clean up the Environment Abstraction Layer (EAL)
>   *
> diff --git a/lib/librte_eal/linux/eal.c b/lib/librte_eal/linux/eal.c
> index 32b48c3de..62747b3a7 100644
> --- a/lib/librte_eal/linux/eal.c
> +++ b/lib/librte_eal/linux/eal.c
> @@ -1160,7 +1160,7 @@ rte_eal_init(int argc, char **argv)
>  #endif
>         }
>
> -       if (rte_eal_log_init(logid, internal_conf->syslog_facility) < 0) {
> +       if (rte_eal_log_init(logid, internal_conf->syslog_facility, NULL) < 0) {
>                 rte_eal_init_alert("Cannot init logging.");
>                 rte_errno = ENOMEM;
>                 __atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
> diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
> index 43c8460bf..f985b3d36 100644
> --- a/lib/librte_eal/linux/eal_log.c
> +++ b/lib/librte_eal/linux/eal_log.c
> @@ -37,18 +37,21 @@ console_log_write(__rte_unused void *c, const char *buf, size_t size)
>         return ret;
>  }
>
> -static cookie_io_functions_t console_log_func = {
> -       .write = console_log_write,
> -};
> -
>  /*
>   * set the log to default function, called during eal init process,
>   * once memzones are available.
>   */
>  int
> -rte_eal_log_init(const char *id, int facility)
> +rte_eal_log_init(const char *id, int facility, rte_log_func_t *logf)
>  {
>         FILE *log_stream;
> +       cookie_io_functions_t console_log_func;
> +
> +       if (logf) {
> +               console_log_func.write = logf;
> +       } else {
> +               console_log_func.write = console_log_write;
> +       }
>
>         log_stream = fopencookie(NULL, "w+", console_log_func);
>         if (log_stream == NULL)
> diff --git a/lib/librte_eal/windows/eal.c b/lib/librte_eal/windows/eal.c
> index 1e5f6576f..74fdd076a 100644
> --- a/lib/librte_eal/windows/eal.c
> +++ b/lib/librte_eal/windows/eal.c
> @@ -266,7 +266,7 @@ rte_eal_init(int argc, char **argv)
>                 eal_get_internal_configuration();
>         int ret;
>
> -       rte_eal_log_init(NULL, 0);
> +       rte_eal_log_init(NULL, 0, NULL);
>
>         eal_log_level_parse(argc, argv);
>
> diff --git a/lib/librte_eal/windows/eal_log.c b/lib/librte_eal/windows/eal_log.c
> index 875981f13..31f556908 100644
> --- a/lib/librte_eal/windows/eal_log.c
> +++ b/lib/librte_eal/windows/eal_log.c
> @@ -6,7 +6,7 @@
>
>  /* set the log to default function, called during eal init process. */
>  int
> -rte_eal_log_init(__rte_unused const char *id, __rte_unused int facility)
> +rte_eal_log_init(__rte_unused const char *id, __rte_unused int facility, rte_log_func_t *logf)
>  {
>         rte_openlog_stream(stderr);
>
> --
> 2.29.2
>

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

[David Marchand]

On Fri, Feb 5, 2021 at 12:25 PM Li Feng <fengli@smartx.com> wrote:
>
> Currently, the dpdk log is out to stdout/stderr and syslog.
> We should support to output the log to another please, e.g. file or
> glog.

Why not use rte_openlog_stream() / rte_log_get_stream() ?


-- 
David Marchand

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

[Feng Li]

David Marchand <david.marchand@redhat.com> 于2021年2月5日周五 下午7:56写道：
>
> On Fri, Feb 5, 2021 at 12:25 PM Li Feng <fengli@smartx.com> wrote:
> >
> > Currently, the dpdk log is out to stdout/stderr and syslog.
> > We should support to output the log to another please, e.g. file or
> > glog.
>
> Why not use rte_openlog_stream() / rte_log_get_stream() ?
>
>
> --
> David Marchand
>
rte_openlog_stream seems it could set an external FILE* stream.
However, in the glog or C++ io stream, it's stream like std::cout.
I think it's hard to use a FILE* stream to unify them.

Use a custom logger function may be a better choice.

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

[Dmitry Kozlyuk]

On Fri, 5 Feb 2021 20:22:36 +0800, Feng Li wrote:
> David Marchand <david.marchand@redhat.com> 于2021年2月5日周五 下午7:56写道：
> >
> > On Fri, Feb 5, 2021 at 12:25 PM Li Feng <fengli@smartx.com> wrote:  
> > >
> > > Currently, the dpdk log is out to stdout/stderr and syslog.
> > > We should support to output the log to another please, e.g. file or
> > > glog.  
> >
> > Why not use rte_openlog_stream() / rte_log_get_stream() ?
> >
> >
> > --
> > David Marchand
> >  
> rte_openlog_stream seems it could set an external FILE* stream.
> However, in the glog or C++ io stream, it's stream like std::cout.
> I think it's hard to use a FILE* stream to unify them.

Also, FILE* is not a language-agnostic interface. Rust/D/etc consumers can't
use this API unless they obtain a FILE* from a compatible libc.

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

[Stephen Hemminger]

On Fri, 5 Feb 2021 12:55:48 +0100
David Marchand <david.marchand@redhat.com> wrote:

> On Fri, Feb 5, 2021 at 12:25 PM Li Feng <fengli@smartx.com> wrote:
> >
> > Currently, the dpdk log is out to stdout/stderr and syslog.
> > We should support to output the log to another please, e.g. file or
> > glog.  
> 
> Why not use rte_openlog_stream() / rte_log_get_stream() ?

Agree, I have used rte_openlog_stream several times to do this.

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

[Stephen Hemminger]

On Fri, 5 Feb 2021 20:22:36 +0800
Feng Li <lifeng1519@gmail.com> wrote:

> David Marchand <david.marchand@redhat.com> 于2021年2月5日周五 下午7:56写道：
> >
> > On Fri, Feb 5, 2021 at 12:25 PM Li Feng <fengli@smartx.com> wrote:  
> > >
> > > Currently, the dpdk log is out to stdout/stderr and syslog.
> > > We should support to output the log to another please, e.g. file or
> > > glog.  
> >
> > Why not use rte_openlog_stream() / rte_log_get_stream() ?
> >
> >
> > --
> > David Marchand
> >  
> rte_openlog_stream seems it could set an external FILE* stream.
> However, in the glog or C++ io stream, it's stream like std::cout.
> I think it's hard to use a FILE* stream to unify them.
> 
> Use a custom logger function may be a better choice.

A FILE * stream can be created that uses any other function under
the covers. See fopencookie(3)

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

[Feng Li]

Stephen Hemminger <stephen@networkplumber.org> 于2021年2月6日周六 上午12:11写道：
>
> On Fri, 5 Feb 2021 12:55:48 +0100
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Fri, Feb 5, 2021 at 12:25 PM Li Feng <fengli@smartx.com> wrote:
> > >
> > > Currently, the dpdk log is out to stdout/stderr and syslog.
> > > We should support to output the log to another please, e.g. file or
> > > glog.
> >
> > Why not use rte_openlog_stream() / rte_log_get_stream() ?
>
> Agree, I have used rte_openlog_stream several times to do this.
As Dmitry Kozlyuk said, FILE* is convenient for the C/libc, but not
for other languages.
C++ standard IO stream is the case.

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

[Feng Li]

Feng Li <lifeng1519@gmail.com> 于2021年2月6日周六 上午12:54写道：
>
> Stephen Hemminger <stephen@networkplumber.org> 于2021年2月6日周六 上午12:11写道：
> >
> > On Fri, 5 Feb 2021 12:55:48 +0100
> > David Marchand <david.marchand@redhat.com> wrote:
> >
> > > On Fri, Feb 5, 2021 at 12:25 PM Li Feng <fengli@smartx.com> wrote:
> > > >
> > > > Currently, the dpdk log is out to stdout/stderr and syslog.
> > > > We should support to output the log to another please, e.g. file or
> > > > glog.
> > >
> > > Why not use rte_openlog_stream() / rte_log_get_stream() ?
> >
> > Agree, I have used rte_openlog_stream several times to do this.
> As Dmitry Kozlyuk said, FILE* is convenient for the C/libc, but not
> for other languages.
> C++ standard IO stream is the case.

The usage is like this:
| +static ssize_t eal_log_func(void* c, const char* buf, size_t size) {
| +    int glog_level = google::GLOG_INFO;
| +    (void)c;
| +    google::LogMessage(__FILE__, __LINE__, glog_level).stream()
| +        << "[DPDK/" << __FUNCTION__ << "] " << std::string(buf, size);
| +    return 0;
| +}

[dpdk-dev] [PATCH v2] log: support custom log function

[Li Feng]

Currently, 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 method is another choice for this scenario.

Change-Id: I3b2419cc2fe5256205daa8077fd8862a8e58fb6c
Signed-off-by: Li Feng <fengli@smartx.com>
---
v2: simplify the code.

 lib/librte_eal/include/rte_eal.h | 16 ++++++++++++++++
 lib/librte_eal/linux/eal_log.c   | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
index eaf6469e5..bd6cf554b 100644
--- a/lib/librte_eal/include/rte_eal.h
+++ b/lib/librte_eal/include/rte_eal.h
@@ -501,6 +501,22 @@ rte_eal_mbuf_user_pool_ops(void);
 const char *
 rte_eal_get_runtime_dir(void);
 
+/**
+ * Usage function typedef used by the application usage function.
+ *
+ * Use this function typedef to define a logger formatter.
+ */
+typedef cookie_write_function_t rte_log_func_t;
+
+/**
+ * Set a customized logger.
+ *
+ * @param logf
+ *   The customized logger function.
+ */
+void
+rte_eal_set_log_func(rte_log_func_t *logf);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
index 43c8460bf..c0a7a12ab 100644
--- a/lib/librte_eal/linux/eal_log.c
+++ b/lib/librte_eal/linux/eal_log.c
@@ -60,3 +60,13 @@ rte_eal_log_init(const char *id, int facility)
 
 	return 0;
 }
+
+/*
+ * Set the customized logger, it will override the default action, which is
+ * writing to syslog and stdout.
+ */
+void
+rte_eal_set_log_func(rte_log_func_t *logf)
+{
+    console_log_func.write = logf;
+}
-- 
2.29.2

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

[Feng Li]

Li Feng <fengli@smartx.com> 于2021年2月6日周六 上午1:42写道：
>
> Currently, 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 method is another choice for this scenario.
>
> Change-Id: I3b2419cc2fe5256205daa8077fd8862a8e58fb6c
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v2: simplify the code.
>
>  lib/librte_eal/include/rte_eal.h | 16 ++++++++++++++++
>  lib/librte_eal/linux/eal_log.c   | 10 ++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> index eaf6469e5..bd6cf554b 100644
> --- a/lib/librte_eal/include/rte_eal.h
> +++ b/lib/librte_eal/include/rte_eal.h
> @@ -501,6 +501,22 @@ rte_eal_mbuf_user_pool_ops(void);
>  const char *
>  rte_eal_get_runtime_dir(void);
>
> +/**
> + * Usage function typedef used by the application usage function.
> + *
> + * Use this function typedef to define a logger formatter.
> + */
> +typedef cookie_write_function_t rte_log_func_t;
> +
> +/**
> + * Set a customized logger.
> + *
> + * @param logf
> + *   The customized logger function.
> + */
> +void
> +rte_eal_set_log_func(rte_log_func_t *logf);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
> index 43c8460bf..c0a7a12ab 100644
> --- a/lib/librte_eal/linux/eal_log.c
> +++ b/lib/librte_eal/linux/eal_log.c
> @@ -60,3 +60,13 @@ rte_eal_log_init(const char *id, int facility)
>
>         return 0;
>  }
> +
> +/*
> + * Set the customized logger, it will override the default action, which is
> + * writing to syslog and stdout.
> + */
> +void
> +rte_eal_set_log_func(rte_log_func_t *logf)
> +{
> +    console_log_func.write = logf;
> +}
> --
> 2.29.2
>
Add more CCers.

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

[Dmitry Kozlyuk]

On Sat,  6 Feb 2021 01:42:04 +0800, Li Feng wrote:
> Currently, 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 method is another choice for this scenario.

It's "DPDK"; and "hard" sounds vague. I'd state the real issue such that
API to change DPDK log sink mandates the (direct) use of libc in consumer
application. To invoke arbitrary function, consumers have to use facilities
that are non-standard (fopencookie) or OS-specific (pipes).

> 
> Change-Id: I3b2419cc2fe5256205daa8077fd8862a8e58fb6c

What is this?

> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v2: simplify the code.
> 
>  lib/librte_eal/include/rte_eal.h | 16 ++++++++++++++++
>  lib/librte_eal/linux/eal_log.c   | 10 ++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> index eaf6469e5..bd6cf554b 100644
> --- a/lib/librte_eal/include/rte_eal.h
> +++ b/lib/librte_eal/include/rte_eal.h
> @@ -501,6 +501,22 @@ rte_eal_mbuf_user_pool_ops(void);
>  const char *
>  rte_eal_get_runtime_dir(void);
>  
> +/**
> + * Usage function typedef used by the application usage function.

It's unrelated to the following typedef purpose, is it?

> + *
> + * Use this function typedef to define a logger formatter.
> + */
> +typedef cookie_write_function_t rte_log_func_t;

"cookie_write_function_t" is not standard C, please write this type
explicitly. POSIX reserves "_t" suffix, "rte_" prefix is enough.

> +
> +/**
> + * Set a customized logger.
> + *
> + * @param logf
> + *   The customized logger function.
> + */
> +void
> +rte_eal_set_log_func(rte_log_func_t *logf);
> +

How about rte_log_sink_set() and a companion rte_log_sink_get() or returning
the previous value when setting a new one? Also, like in fopencookie(), a
cookie argument is needed.

New functions must be marker __rte_experimental and added to .map and .def
files. Please use devtools/checkpatches.sh that will run checks like that for
your commits.

>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
> index 43c8460bf..c0a7a12ab 100644
> --- a/lib/librte_eal/linux/eal_log.c
> +++ b/lib/librte_eal/linux/eal_log.c
> @@ -60,3 +60,13 @@ rte_eal_log_init(const char *id, int facility)
>  
>  	return 0;
>  }
> +
> +/*
> + * Set the customized logger, it will override the default action, which is
> + * writing to syslog and stdout.
> + */
> +void
> +rte_eal_set_log_func(rte_log_func_t *logf)
> +{
> +    console_log_func.write = logf;
> +}

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

[Feng Li]

Thanks for your comments.

Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> 于2021年2月6日周六 上午3:32写道：
>
> On Sat,  6 Feb 2021 01:42:04 +0800, Li Feng wrote:
> > Currently, 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 method is another choice for this scenario.
>
> It's "DPDK"; and "hard" sounds vague. I'd state the real issue such that
> API to change DPDK log sink mandates the (direct) use of libc in consumer
> application. To invoke arbitrary function, consumers have to use facilities
> that are non-standard (fopencookie) or OS-specific (pipes).
>
> >
> > Change-Id: I3b2419cc2fe5256205daa8077fd8862a8e58fb6c
>
> What is this?
>
It's Gerrit's Change-ID, I will remove it.

> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v2: simplify the code.
> >
> >  lib/librte_eal/include/rte_eal.h | 16 ++++++++++++++++
> >  lib/librte_eal/linux/eal_log.c   | 10 ++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> > index eaf6469e5..bd6cf554b 100644
> > --- a/lib/librte_eal/include/rte_eal.h
> > +++ b/lib/librte_eal/include/rte_eal.h
> > @@ -501,6 +501,22 @@ rte_eal_mbuf_user_pool_ops(void);
> >  const char *
> >  rte_eal_get_runtime_dir(void);
> >
> > +/**
> > + * Usage function typedef used by the application usage function.
>
> It's unrelated to the following typedef purpose, is it?
It's borrowed from the front typedef sentence.

>
> > + *
> > + * Use this function typedef to define a logger formatter.
> > + */
> > +typedef cookie_write_function_t rte_log_func_t;
>
> "cookie_write_function_t" is not standard C, please write this type
> explicitly. POSIX reserves "_t" suffix, "rte_" prefix is enough.
Fix to this?
typedef cookie_write_function_t rte_cookie_write_function;

>
> > +
> > +/**
> > + * Set a customized logger.
> > + *
> > + * @param logf
> > + *   The customized logger function.
> > + */
> > +void
> > +rte_eal_set_log_func(rte_log_func_t *logf);
> > +
>
> How about rte_log_sink_set() and a companion rte_log_sink_get() or returning
> the previous value when setting a new one? Also, like in fopencookie(), a
> cookie argument is needed.

void rte_log_sink_set(rte_cookie_write_function* cookie_write);
rte_cookie_write_function* rte_log_sink_get();
Right?

>
> New functions must be marker __rte_experimental and added to .map and .def
> files. Please use devtools/checkpatches.sh that will run checks like that for
> your commits.
>
I have run the `devtools/checkpatches.sh`, it reports valid, so weird.
I'll fix it.


> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
> > index 43c8460bf..c0a7a12ab 100644
> > --- a/lib/librte_eal/linux/eal_log.c
> > +++ b/lib/librte_eal/linux/eal_log.c
> > @@ -60,3 +60,13 @@ rte_eal_log_init(const char *id, int facility)
> >
> >       return 0;
> >  }
> > +
> > +/*
> > + * Set the customized logger, it will override the default action, which is
> > + * writing to syslog and stdout.
> > + */
> > +void
> > +rte_eal_set_log_func(rte_log_func_t *logf)
> > +{
> > +    console_log_func.write = logf;
> > +}
>

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

[Dmitry Kozlyuk]

On Mon, 8 Feb 2021 14:58:29 +0800, Feng Li wrote:
> > > +/**
> > > + * Usage function typedef used by the application usage function.  
> >
> > It's unrelated to the following typedef purpose, is it?  
> It's borrowed from the front typedef sentence.

Doesn't make much sense here anyway.

> > > + *
> > > + * Use this function typedef to define a logger formatter.
> > > + */
> > > +typedef cookie_write_function_t rte_log_func_t;  
> >
> > "cookie_write_function_t" is not standard C, please write this type
> > explicitly. POSIX reserves "_t" suffix, "rte_" prefix is enough.  
> Fix to this?
> typedef cookie_write_function_t rte_cookie_write_function;

You cannot expose "cookie_write_function_t" in public API, because it is not
portable. You have to write out the type in full.
I'd replace "cookie" with "log".

> void rte_log_sink_set(rte_cookie_write_function* cookie_write);
> rte_cookie_write_function* rte_log_sink_get();
> Right?

OK.

> > > +void
> > > +rte_eal_set_log_func(rte_log_func_t *logf)
> > > +{
> > > +    console_log_func.write = logf;
> > > +}

Is this correct? AFAIK, updating the member has no effect, unless you call
fopencookie() with an updated structure.

Your new callbacks don't seem to be integrated with DPDK logging system for
all platforms ("eal_common_log.c", "windows/eal_log.c"). If the new API and
rte_openlog_stream() cancel effect of each other, this should be documented.

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

[Feng Li]

On Tue, Feb 9, 2021 at 6:40 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> On Mon, 8 Feb 2021 14:58:29 +0800, Feng Li wrote:
> > > > +/**
> > > > + * Usage function typedef used by the application usage function.
> > >
> > > It's unrelated to the following typedef purpose, is it?
> > It's borrowed from the front typedef sentence.
>
> Doesn't make much sense here anyway.
Acked.

>
> > > > + *
> > > > + * Use this function typedef to define a logger formatter.
> > > > + */
> > > > +typedef cookie_write_function_t rte_log_func_t;
> > >
> > > "cookie_write_function_t" is not standard C, please write this type
> > > explicitly. POSIX reserves "_t" suffix, "rte_" prefix is enough.
> > Fix to this?
> > typedef cookie_write_function_t rte_cookie_write_function;
>
> You cannot expose "cookie_write_function_t" in public API, because it is not
> portable. You have to write out the type in full.
> I'd replace "cookie" with "log".
Acked.

>
> > void rte_log_sink_set(rte_cookie_write_function* cookie_write);
> > rte_cookie_write_function* rte_log_sink_get();
> > Right?
>
> OK.
>
> > > > +void
> > > > +rte_eal_set_log_func(rte_log_func_t *logf)
> > > > +{
> > > > +    console_log_func.write = logf;
> > > > +}
>
> Is this correct? AFAIK, updating the member has no effect, unless you call
> fopencookie() with an updated structure.
The call order is:
rte_eal_set_log_func(logf);
...
rte_eal_init();

Because the rte_eal_init will call rte_eal_log_init, so set the struct
before rte_eal_init.
And the logs in `rte_eal_init` will also be redirected.

>
> Your new callbacks don't seem to be integrated with DPDK logging system for
> all platforms ("eal_common_log.c", "windows/eal_log.c"). If the new API and
> rte_openlog_stream() cancel effect of each other, this should be documented.
>

Looks like `rte_eal_log_init` is defined in Linux and Windows, and
Windows only redirects
the log to stderr. So Windows doesn't support this feature.

rte_openlog_stream has a higher priority than rte_eal_set_log_func.
If it isn't called, then 'rte_eal_set_log_func' call will work.
I will update the comments of this API.

Thanks.

[dpdk-dev] [PATCH v3] log: support custom log function

[Li Feng]

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>
---
v3: Rename the func, change the comments, add funcs in version.map.
v2: Simplify the code.

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

diff --git a/lib/librte_eal/include/rte_log.h b/lib/librte_eal/include/rte_log.h
index 173004fd7..566319cd4 100644
--- a/lib/librte_eal/include/rte_log.h
+++ b/lib/librte_eal/include/rte_log.h
@@ -97,6 +97,35 @@ 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..2192b43f6 100644
--- a/lib/librte_eal/linux/eal_log.c
+++ b/lib/librte_eal/linux/eal_log.c
@@ -60,3 +60,24 @@ 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..3cc8691a3 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..42c2d5ff3 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);
+	return;
+}
+
+/*
+ * Retrieve the default log write function.
+ */
+rte_log_write_function*
+rte_log_sink_get(void) {
+    return NULL;
+}
-- 
2.29.2

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

[Feng Li]

Ping……
Any comments about this?

Thanks,

On Wed, Feb 10, 2021 at 1:20 PM Li Feng <fengli@smartx.com> 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>
> ---
> v3: Rename the func, change the comments, add funcs in version.map.
> v2: Simplify the code.
>
>  lib/librte_eal/include/rte_log.h | 29 +++++++++++++++++++++++++++++
>  lib/librte_eal/linux/eal_log.c   | 21 +++++++++++++++++++++
>  lib/librte_eal/version.map       |  2 ++
>  lib/librte_eal/windows/eal_log.c | 19 +++++++++++++++++++
>  4 files changed, 71 insertions(+)
>
> diff --git a/lib/librte_eal/include/rte_log.h b/lib/librte_eal/include/rte_log.h
> index 173004fd7..566319cd4 100644
> --- a/lib/librte_eal/include/rte_log.h
> +++ b/lib/librte_eal/include/rte_log.h
> @@ -97,6 +97,35 @@ 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..2192b43f6 100644
> --- a/lib/librte_eal/linux/eal_log.c
> +++ b/lib/librte_eal/linux/eal_log.c
> @@ -60,3 +60,24 @@ 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..3cc8691a3 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..42c2d5ff3 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);
> +       return;
> +}
> +
> +/*
> + * Retrieve the default log write function.
> + */
> +rte_log_write_function*
> +rte_log_sink_get(void) {
> +    return NULL;
> +}
> --
> 2.29.2
>

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

[Li Feng]

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(+)

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;
+}
-- 
2.29.2

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

[Dmitry Kozlyuk]

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.

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

[Feng Li]

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.

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

[Stephen Hemminger]

On Wed, 10 Feb 2021 13:20:19 +0800
Li Feng <fengli@smartx.com> 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>
> ---

Rejecting this patch.
You can do the same thing with a small bit of C called during application
init that creates a standard I/O stream. Like fopencookie().

The added overhead of adding callback here adds unnecessary complexity.