diff mbox series

[v2] log: support custom log function

Message ID 20210205174204.1878089-1-fengli@smartx.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] log: support custom log function |

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed
ci/iol-testing fail Testing issues
ci/checkpatch warning coding style issues

Commit Message

Li Feng Feb. 5, 2021, 5:42 p.m. UTC 
  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(+)

Comments

Feng Li Feb. 5, 2021, 5:47 p.m. UTC | #1 
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.
Dmitry Kozlyuk Feb. 5, 2021, 7:32 p.m. UTC | #2 
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;
> +}
Feng Li Feb. 8, 2021, 6:58 a.m. UTC | #3 
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;
> > +}
>
Dmitry Kozlyuk Feb. 8, 2021, 10:40 p.m. UTC | #4 
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.
Feng Li Feb. 10, 2021, 3:59 a.m. UTC | #5 
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.
diff mbox series

Patch

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