DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] log: support custom log function
@ 2021-02-05 11:24 Li Feng
  2021-02-05 11:31 ` Feng Li
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Li Feng @ 2021-02-05 11:24 UTC (permalink / raw)
  To: Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy, Pallavi Kadam
  Cc: dev, lifeng1519, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 11:24 [dpdk-dev] [PATCH] log: support custom log function Li Feng
@ 2021-02-05 11:31 ` Feng Li
  2021-02-05 11:55 ` David Marchand
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 20+ messages in thread
From: Feng Li @ 2021-02-05 11:31 UTC (permalink / raw)
  To: Li Feng
  Cc: Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, dev

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
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 11:24 [dpdk-dev] [PATCH] log: support custom log function Li Feng
  2021-02-05 11:31 ` Feng Li
@ 2021-02-05 11:55 ` David Marchand
  2021-02-05 12:22   ` Feng Li
  2021-02-05 16:10   ` Stephen Hemminger
  2021-02-05 17:42 ` [dpdk-dev] [PATCH v2] " Li Feng
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 20+ messages in thread
From: David Marchand @ 2021-02-05 11:55 UTC (permalink / raw)
  To: Li Feng
  Cc: Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, dev, Feng Li

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 11:55 ` David Marchand
@ 2021-02-05 12:22   ` Feng Li
  2021-02-05 14:08     ` Dmitry Kozlyuk
  2021-02-05 16:39     ` Stephen Hemminger
  2021-02-05 16:10   ` Stephen Hemminger
  1 sibling, 2 replies; 20+ messages in thread
From: Feng Li @ 2021-02-05 12:22 UTC (permalink / raw)
  To: David Marchand
  Cc: Li Feng, Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, dev

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 12:22   ` Feng Li
@ 2021-02-05 14:08     ` Dmitry Kozlyuk
  2021-02-05 16:39     ` Stephen Hemminger
  1 sibling, 0 replies; 20+ messages in thread
From: Dmitry Kozlyuk @ 2021-02-05 14:08 UTC (permalink / raw)
  To: Feng Li
  Cc: David Marchand, Li Feng, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, dev

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 11:55 ` David Marchand
  2021-02-05 12:22   ` Feng Li
@ 2021-02-05 16:10   ` Stephen Hemminger
  2021-02-05 16:54     ` Feng Li
  1 sibling, 1 reply; 20+ messages in thread
From: Stephen Hemminger @ 2021-02-05 16:10 UTC (permalink / raw)
  To: David Marchand
  Cc: Li Feng, Dmitry Kozlyuk, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, dev, Feng Li

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 12:22   ` Feng Li
  2021-02-05 14:08     ` Dmitry Kozlyuk
@ 2021-02-05 16:39     ` Stephen Hemminger
  1 sibling, 0 replies; 20+ messages in thread
From: Stephen Hemminger @ 2021-02-05 16:39 UTC (permalink / raw)
  To: Feng Li
  Cc: David Marchand, Li Feng, Dmitry Kozlyuk,
	Narcisa Ana Maria Vasile, Dmitry Malloy, Pallavi Kadam, dev

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)


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 16:10   ` Stephen Hemminger
@ 2021-02-05 16:54     ` Feng Li
  2021-02-05 17:06       ` Feng Li
  0 siblings, 1 reply; 20+ messages in thread
From: Feng Li @ 2021-02-05 16:54 UTC (permalink / raw)
  To: stephen
  Cc: David Marchand, Li Feng, Dmitry Kozlyuk,
	Narcisa Ana Maria Vasile, Dmitry Malloy, Pallavi Kadam, dev

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] log: support custom log function
  2021-02-05 16:54     ` Feng Li
@ 2021-02-05 17:06       ` Feng Li
  0 siblings, 0 replies; 20+ messages in thread
From: Feng Li @ 2021-02-05 17:06 UTC (permalink / raw)
  To: stephen
  Cc: David Marchand, Li Feng, Dmitry Kozlyuk,
	Narcisa Ana Maria Vasile, Dmitry Malloy, Pallavi Kadam, dev

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v2] log: support custom log function
  2021-02-05 11:24 [dpdk-dev] [PATCH] log: support custom log function Li Feng
  2021-02-05 11:31 ` Feng Li
  2021-02-05 11:55 ` David Marchand
@ 2021-02-05 17:42 ` Li Feng
  2021-02-05 17:47   ` Feng Li
  2021-02-05 19:32   ` Dmitry Kozlyuk
  2021-02-10  5:20 ` [dpdk-dev] [PATCH v3] " Li Feng
  2021-02-18  6:12 ` [dpdk-dev] [PATCH v4] " Li Feng
  4 siblings, 2 replies; 20+ messages in thread
From: Li Feng @ 2021-02-05 17:42 UTC (permalink / raw)
  Cc: dev, lifeng1519, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2] log: support custom log function
  2021-02-05 17:42 ` [dpdk-dev] [PATCH v2] " Li Feng
@ 2021-02-05 17:47   ` Feng Li
  2021-02-05 19:32   ` Dmitry Kozlyuk
  1 sibling, 0 replies; 20+ messages in thread
From: Feng Li @ 2021-02-05 17:47 UTC (permalink / raw)
  To: Li Feng, David Marchand, Dmitry Kozlyuk,
	Narcisa Ana Maria Vasile, Dmitry Malloy, Pallavi Kadam
  Cc: dev

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2] log: support custom log function
  2021-02-05 17:42 ` [dpdk-dev] [PATCH v2] " Li Feng
  2021-02-05 17:47   ` Feng Li
@ 2021-02-05 19:32   ` Dmitry Kozlyuk
  2021-02-08  6:58     ` Feng Li
  1 sibling, 1 reply; 20+ messages in thread
From: Dmitry Kozlyuk @ 2021-02-05 19:32 UTC (permalink / raw)
  To: Li Feng; +Cc: dev, lifeng1519

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2] log: support custom log function
  2021-02-05 19:32   ` Dmitry Kozlyuk
@ 2021-02-08  6:58     ` Feng Li
  2021-02-08 22:40       ` Dmitry Kozlyuk
  0 siblings, 1 reply; 20+ messages in thread
From: Feng Li @ 2021-02-08  6:58 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: Li Feng, dev, David Marchand, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2] log: support custom log function
  2021-02-08  6:58     ` Feng Li
@ 2021-02-08 22:40       ` Dmitry Kozlyuk
  2021-02-10  3:59         ` Feng Li
  0 siblings, 1 reply; 20+ messages in thread
From: Dmitry Kozlyuk @ 2021-02-08 22:40 UTC (permalink / raw)
  To: Feng Li
  Cc: Li Feng, dev, David Marchand, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam

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.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2] log: support custom log function
  2021-02-08 22:40       ` Dmitry Kozlyuk
@ 2021-02-10  3:59         ` Feng Li
  0 siblings, 0 replies; 20+ messages in thread
From: Feng Li @ 2021-02-10  3:59 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: Li Feng, dev, David Marchand, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v3] log: support custom log function
  2021-02-05 11:24 [dpdk-dev] [PATCH] log: support custom log function Li Feng
                   ` (2 preceding siblings ...)
  2021-02-05 17:42 ` [dpdk-dev] [PATCH v2] " Li Feng
@ 2021-02-10  5:20 ` Li Feng
  2021-02-18  2:55   ` Feng Li
  2021-02-18  6:12 ` [dpdk-dev] [PATCH v4] " Li Feng
  4 siblings, 1 reply; 20+ messages in thread
From: Li Feng @ 2021-02-10  5:20 UTC (permalink / raw)
  To: David Marchand, Dmitry Kozlyuk, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam, Ray Kinsella, Neil Horman
  Cc: dev, lifeng1519, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v3] log: support custom log function
  2021-02-10  5:20 ` [dpdk-dev] [PATCH v3] " Li Feng
@ 2021-02-18  2:55   ` Feng Li
  0 siblings, 0 replies; 20+ messages in thread
From: Feng Li @ 2021-02-18  2:55 UTC (permalink / raw)
  To: Li Feng
  Cc: David Marchand, Dmitry Kozlyuk, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam, Ray Kinsella, Neil Horman, dev

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
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v4] log: support custom log function
  2021-02-05 11:24 [dpdk-dev] [PATCH] log: support custom log function Li Feng
                   ` (3 preceding siblings ...)
  2021-02-10  5:20 ` [dpdk-dev] [PATCH v3] " Li Feng
@ 2021-02-18  6:12 ` Li Feng
  2021-02-19  8:11   ` Dmitry Kozlyuk
  4 siblings, 1 reply; 20+ messages in thread
From: Li Feng @ 2021-02-18  6:12 UTC (permalink / raw)
  To: David Marchand, Dmitry Kozlyuk, Narcisa Ana Maria Vasile,
	Dmitry Malloy, Pallavi Kadam, Ray Kinsella, Neil Horman
  Cc: dev, lifeng1519, 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v4] log: support custom log function
  2021-02-18  6:12 ` [dpdk-dev] [PATCH v4] " Li Feng
@ 2021-02-19  8:11   ` Dmitry Kozlyuk
  2021-02-23 11:22     ` Feng Li
  0 siblings, 1 reply; 20+ messages in thread
From: Dmitry Kozlyuk @ 2021-02-19  8:11 UTC (permalink / raw)
  To: Li Feng
  Cc: David Marchand, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, Ray Kinsella, Neil Horman, dev, lifeng1519

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v4] log: support custom log function
  2021-02-19  8:11   ` Dmitry Kozlyuk
@ 2021-02-23 11:22     ` Feng Li
  0 siblings, 0 replies; 20+ messages in thread
From: Feng Li @ 2021-02-23 11:22 UTC (permalink / raw)
  To: Dmitry Kozlyuk
  Cc: Li Feng, David Marchand, Narcisa Ana Maria Vasile, Dmitry Malloy,
	Pallavi Kadam, Ray Kinsella, Neil Horman, dev

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-02-24  2:15 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 11:24 [dpdk-dev] [PATCH] log: support custom log function Li Feng
2021-02-05 11:31 ` Feng Li
2021-02-05 11:55 ` David Marchand
2021-02-05 12:22   ` Feng Li
2021-02-05 14:08     ` Dmitry Kozlyuk
2021-02-05 16:39     ` Stephen Hemminger
2021-02-05 16:10   ` Stephen Hemminger
2021-02-05 16:54     ` Feng Li
2021-02-05 17:06       ` Feng Li
2021-02-05 17:42 ` [dpdk-dev] [PATCH v2] " Li Feng
2021-02-05 17:47   ` Feng Li
2021-02-05 19:32   ` Dmitry Kozlyuk
2021-02-08  6:58     ` Feng Li
2021-02-08 22:40       ` Dmitry Kozlyuk
2021-02-10  3:59         ` Feng Li
2021-02-10  5:20 ` [dpdk-dev] [PATCH v3] " Li Feng
2021-02-18  2:55   ` Feng Li
2021-02-18  6:12 ` [dpdk-dev] [PATCH v4] " Li Feng
2021-02-19  8:11   ` Dmitry Kozlyuk
2021-02-23 11:22     ` Feng Li

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git