[dpdk-dev] [PATCH] log: respect rte_openlog_stream calls before rte_eal_init

[dpdk-dev] [PATCH] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

Before this patch, rte_eal_init invoked rte_openlog_stream, cancelling
any application-specific logger and making it it impossible for an
application to capture the initial log messages generated during
rte_eal_init. With this patch, applications can capture all of the
log messages.

Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
---
 lib/librte_eal/bsdapp/eal/eal_log.c     |  3 +--
 lib/librte_eal/common/eal_common_log.c  | 13 +++++++------
 lib/librte_eal/common/include/rte_log.h |  2 +-
 lib/librte_eal/linuxapp/eal/eal_log.c   |  3 +--
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c b/lib/librte_eal/bsdapp/eal/eal_log.c
index a425f7a..c5b1dee 100644
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ b/lib/librte_eal/bsdapp/eal/eal_log.c
@@ -52,6 +52,5 @@ rte_eal_log_init(const char *id __rte_unused, int facility __rte_unused)
 int
 rte_eal_log_early_init(void)
 {
-	rte_openlog_stream(stderr);
-	return 0;
+	return rte_eal_common_log_init(stderr);
 }
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 967991a..cfc4c8b 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -48,11 +48,12 @@ struct rte_logs rte_logs = {
 	.file = NULL,
 };
 
+/* Stream to use for logging if rte_logs.file is NULL */
 static FILE *default_log_stream;
 
 /**
  * This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
  */
 struct log_cur_msg {
 	uint32_t loglevel; /**< log level - see rte_log.h */
@@ -68,10 +69,7 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
 int
 rte_openlog_stream(FILE *f)
 {
-	if (f == NULL)
-		rte_logs.file = default_log_stream;
-	else
-		rte_logs.file = f;
+	rte_logs.file = f;
 	return 0;
 }
 
@@ -127,6 +125,8 @@ rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
 {
 	int ret;
 	FILE *f = rte_logs.file;
+	if (f == NULL)
+		f = default_log_stream;
 
 	if ((level > rte_logs.level) || !(logtype & rte_logs.type))
 		return 0;
@@ -158,7 +158,8 @@ rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
 }
 
 /*
- * called by environment-specific log init function
+ * Called during initialization to specify where to log messages if
+ * openlog_stream hasn't been called.
  */
 int
 rte_eal_common_log_init(FILE *default_log)
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 919563c..d99baf3 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -54,7 +54,7 @@ extern "C" {
 struct rte_logs {
 	uint32_t type;  /**< Bitfield with enabled logs. */
 	uint32_t level; /**< Log level. */
-	FILE *file;     /**< Pointer to current FILE* for logs. */
+	FILE *file;     /**< Output file set by rte_openlog_stream, or NULL. */
 };
 
 /** Global log informations */
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index d391100..aafc41c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -136,6 +136,5 @@ rte_eal_log_early_init(void)
 		printf("Cannot configure early_log_stream\n");
 		return -1;
 	}
-	rte_openlog_stream(early_log_stream);
-	return 0;
+	return rte_eal_common_log_init(early_log_stream);
 }
-- 
2.8.3

Re: [dpdk-dev] [PATCH] log: respect rte_openlog_stream calls before rte_eal_init

[Thomas Monjalon]

2016-09-28 13:42, John Ousterhout:
> Before this patch, rte_eal_init invoked rte_openlog_stream, cancelling
> any application-specific logger and making it it impossible for an
> application to capture the initial log messages generated during
> rte_eal_init. With this patch, applications can capture all of the
> log messages.

It deserves an explanation of what the patch does, i.e. the new logic.
If think you just want to remove the log init from rte_eal_init and
use the default stream when it is not initialized (NULL).

You could also remove the early log stream since the default one could
work from the early stage. Indeed the complex log history was removed:
	http://dpdk.org/commit/a3f34a98

Thanks for improving the usability.

[dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

Before this patch, application-specific loggers could not be
installed before rte_eal_init completed (the initialization process
called rte_openlog_stream, overwriting any previously installed
logger). This made it impossible for an application to capture the
initial log messages generated during rte_eal_init. This patch changes
initialization so that information from a previous call to
rte_openlog_stream is not lost. Specifically:
* The default log stream is now maintained separately from an
  application-specific log stream installed with rte_openlog_stream.
* rte_eal_common_log_init has been renamed to rte_eal_log_set_default,
  since this is all it does. It no longer invokes rte_openlog_stream; it
  just updates the default stream. Also, this method now returns void,
  rather than int, since there are no errors.
* The "early log" mechanism (e.g. rte_eal_log_early_init) has been
  removed; all of the desired functionality can be achieved by calling
  rte_eal_log_set_default.

Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
----
v2:
* Removed the early log mechanism, renamed rte_eal_common_log_init.

Note: I see from the code that Linux and BSD set different default streams:
Linux uses stdout, while BSD uses stderr. This patch retains the distinction,
though I'm not sure why it is there.
---
 lib/librte_eal/bsdapp/eal/eal.c         |  3 +--
 lib/librte_eal/bsdapp/eal/eal_log.c     | 11 +--------
 lib/librte_eal/common/eal_common_log.c  | 19 +++++++---------
 lib/librte_eal/common/eal_private.h     | 16 ++++---------
 lib/librte_eal/common/include/rte_log.h |  2 +-
 lib/librte_eal/linuxapp/eal/eal.c       |  3 +--
 lib/librte_eal/linuxapp/eal/eal_log.c   | 40 +--------------------------------
 7 files changed, 17 insertions(+), 77 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index a0c8f8c..a1ef75b 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -501,8 +501,7 @@ rte_eal_init(int argc, char **argv)
 
 	thread_id = pthread_self();
 
-	if (rte_eal_log_early_init() < 0)
-		rte_panic("Cannot init early logs\n");
+	rte_eal_log_set_default(stderr);
 
 	eal_log_level_parse(argc, argv);
 
diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c b/lib/librte_eal/bsdapp/eal/eal_log.c
index a425f7a..6b2c6da 100644
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ b/lib/librte_eal/bsdapp/eal/eal_log.c
@@ -44,14 +44,5 @@
 int
 rte_eal_log_init(const char *id __rte_unused, int facility __rte_unused)
 {
-	if (rte_eal_common_log_init(stderr) < 0)
-		return -1;
-	return 0;
-}
-
-int
-rte_eal_log_early_init(void)
-{
-	rte_openlog_stream(stderr);
-	return 0;
+	rte_eal_set_default(stderr);
 }
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 967991a..2181cfa 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -48,11 +48,12 @@ struct rte_logs rte_logs = {
 	.file = NULL,
 };
 
+/* Stream to use for logging if rte_logs.file is NULL */
 static FILE *default_log_stream;
 
 /**
  * This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
  */
 struct log_cur_msg {
 	uint32_t loglevel; /**< log level - see rte_log.h */
@@ -68,10 +69,7 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
 int
 rte_openlog_stream(FILE *f)
 {
-	if (f == NULL)
-		rte_logs.file = default_log_stream;
-	else
-		rte_logs.file = f;
+	rte_logs.file = f;
 	return 0;
 }
 
@@ -127,6 +125,8 @@ rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
 {
 	int ret;
 	FILE *f = rte_logs.file;
+	if (f == NULL)
+		f = default_log_stream;
 
 	if ((level > rte_logs.level) || !(logtype & rte_logs.type))
 		return 0;
@@ -158,17 +158,14 @@ rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
 }
 
 /*
- * called by environment-specific log init function
+ * Called by environment-specific initialization functions.
  */
-int
-rte_eal_common_log_init(FILE *default_log)
+void
+rte_eal_log_set_default(FILE *default_log)
 {
 	default_log_stream = default_log;
-	rte_openlog_stream(default_log);
 
 #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
 	RTE_LOG(NOTICE, EAL, "Debug logs available - lower performance\n");
 #endif
-
-	return 0;
 }
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 19f7535..a037994 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -47,7 +47,9 @@
 int rte_eal_memzone_init(void);
 
 /**
- * Common log initialization function (private to eal).
+ * Common log initialization function (private to eal).  Determines
+ * where log data is written when no call to eal_openlog_stream is
+ * in effect.
  *
  * @param default_log
  *   The default log stream to be used.
@@ -55,7 +57,7 @@ int rte_eal_memzone_init(void);
  *   - 0 on success
  *   - Negative on error
  */
-int rte_eal_common_log_init(FILE *default_log);
+void rte_eal_log_set_default(FILE *default_log);
 
 /**
  * Fill configuration with number of physical and logical processors
@@ -97,16 +99,6 @@ int rte_eal_memory_init(void);
 int rte_eal_timer_init(void);
 
 /**
- * Init early logs
- *
- * This function is private to EAL.
- *
- * @return
- *   0 on success, negative on error
- */
-int rte_eal_log_early_init(void);
-
-/**
  * Init the default log stream
  *
  * This function is private to EAL.
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 919563c..d99baf3 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -54,7 +54,7 @@ extern "C" {
 struct rte_logs {
 	uint32_t type;  /**< Bitfield with enabled logs. */
 	uint32_t level; /**< Log level. */
-	FILE *file;     /**< Pointer to current FILE* for logs. */
+	FILE *file;     /**< Output file set by rte_openlog_stream, or NULL. */
 };
 
 /** Global log informations */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index d5b81a3..4bd2439 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -748,8 +748,7 @@ rte_eal_init(int argc, char **argv)
 
 	thread_id = pthread_self();
 
-	if (rte_eal_log_early_init() < 0)
-		rte_panic("Cannot init early logs\n");
+	rte_eal_log_set_default(stdout);
 
 	eal_log_level_parse(argc, argv);
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index d391100..4a6690a 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -97,45 +97,7 @@ rte_eal_log_init(const char *id, int facility)
 
 	openlog(id, LOG_NDELAY | LOG_PID, facility);
 
-	if (rte_eal_common_log_init(log_stream) < 0)
-		return -1;
-
-	return 0;
-}
-
-/* early logs */
-
-/*
- * early log function, used before rte_eal_log_init
- */
-static ssize_t
-early_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
-{
-	ssize_t ret;
-	ret = fwrite(buf, size, 1, stdout);
-	fflush(stdout);
-	if (ret == 0)
-		return -1;
-	return ret;
-}
-
-static cookie_io_functions_t early_log_func = {
-	.write = early_log_write,
-};
-static FILE *early_log_stream;
+	rte_eal_log_set_default(log_stream);
 
-/*
- * init the log library, called by rte_eal_init() to enable early
- * logs
- */
-int
-rte_eal_log_early_init(void)
-{
-	early_log_stream = fopencookie(NULL, "w+", early_log_func);
-	if (early_log_stream == NULL) {
-		printf("Cannot configure early_log_stream\n");
-		return -1;
-	}
-	rte_openlog_stream(early_log_stream);
 	return 0;
 }
-- 
2.8.3

Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[Thomas Monjalon]

2016-10-10 15:39, John Ousterhout:
> Before this patch, application-specific loggers could not be
> installed before rte_eal_init completed (the initialization process
> called rte_openlog_stream, overwriting any previously installed
> logger). This made it impossible for an application to capture the
> initial log messages generated during rte_eal_init. This patch changes
> initialization so that information from a previous call to
> rte_openlog_stream is not lost. Specifically:
> * The default log stream is now maintained separately from an
>   application-specific log stream installed with rte_openlog_stream.
> * rte_eal_common_log_init has been renamed to rte_eal_log_set_default,
>   since this is all it does. It no longer invokes rte_openlog_stream; it
>   just updates the default stream. Also, this method now returns void,
>   rather than int, since there are no errors.
> * The "early log" mechanism (e.g. rte_eal_log_early_init) has been
>   removed; all of the desired functionality can be achieved by calling
>   rte_eal_log_set_default.
> 
> Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
> ----
> v2:
> * Removed the early log mechanism, renamed rte_eal_common_log_init.
> 
> Note: I see from the code that Linux and BSD set different default streams:
> Linux uses stdout, while BSD uses stderr. This patch retains the distinction,
> though I'm not sure why it is there.

I don't know either.
What is best between stdout and stderr for logs?

[...]
> -int
> -rte_eal_log_early_init(void)
> -{
> -	rte_openlog_stream(stderr);
> -	return 0;
> +	rte_eal_set_default(stderr);

It should be rte_eal_log_set_default.

[...]
>  /*
> - * called by environment-specific log init function
> + * Called by environment-specific initialization functions.
>   */
> -int
> -rte_eal_common_log_init(FILE *default_log)
> +void
> +rte_eal_log_set_default(FILE *default_log)
>  {
>  	default_log_stream = default_log;
> -	rte_openlog_stream(default_log);
>  
>  #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
>  	RTE_LOG(NOTICE, EAL, "Debug logs available - lower performance\n");
>  #endif
> -
> -	return 0;
>  }

Do we really need a function for that?
Why not just initialize default_log_stream statically?

[...]
>  /**
> - * Common log initialization function (private to eal).
> + * Common log initialization function (private to eal).  Determines
> + * where log data is written when no call to eal_openlog_stream is
> + * in effect.

It should be rte_openlog_stream.

Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:
>
> 2016-10-10 15:39, John Ousterhout:
> > ...
> >
> > Note: I see from the code that Linux and BSD set different default
streams:
> > Linux uses stdout, while BSD uses stderr. This patch retains the
distinction,
> > though I'm not sure why it is there.
>
> I don't know either.
> What is best between stdout and stderr for logs?

I would guess that stdout makes more sense, since most log entries describe
normal operation, not errors. I'm happy to make these consistent, but this
would introduce a behavior change for BSD (which currently uses stderr);
would that be considered antisocial?

> [...]
> > -int
> > -rte_eal_log_early_init(void)
> > -{
> > -     rte_openlog_stream(stderr);
> > -     return 0;
> > +     rte_eal_set_default(stderr);
>
> It should be rte_eal_log_set_default.

Oops, right; will fix.

>
> [...]
> >  /*
> > - * called by environment-specific log init function
> > + * Called by environment-specific initialization functions.
> >   */
> > -int
> > -rte_eal_common_log_init(FILE *default_log)
> > +void
> > +rte_eal_log_set_default(FILE *default_log)
> >  {
> >       default_log_stream = default_log;
> > -     rte_openlog_stream(default_log);
> >
> >  #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
> >       RTE_LOG(NOTICE, EAL, "Debug logs available - lower
performance\n");
> >  #endif
> > -
> > -     return 0;
> >  }
>
> Do we really need a function for that?
> Why not just initialize default_log_stream statically?

Right now, different platforms have different defaults. BSD uses stderr
always. Linux starts out with stdout as the default, but later during
initialization it switches to a different default that logs messages both
to  standard output and also to syslog. I don't have enough experience with
DPDK to know whether a single approach is really right for all systems (or
which approach it should be), and since I'm a DPDK newbie I thought it best
to take a more conservative approach and avoid behavioral changes. My
personal preference would be to minimize mission creep with this patch and
leave that behavior in place for someone with more expertise to deal with
later (and this issue is orthogonal to the main goal of the patch). But, if
unifying the log defaults is considered essential to the patch (and is
noncontroversial), I'm willing to implement it.

> [...]
> >  /**
> > - * Common log initialization function (private to eal).
> > + * Common log initialization function (private to eal).  Determines
> > + * where log data is written when no call to eal_openlog_stream is
> > + * in effect.
>
> It should be rte_openlog_stream.

Oops; fixed.

Thanks for the comments.

-John-

Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[Thomas Monjalon]

2016-10-11 09:30, John Ousterhout:
> On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> >
> > 2016-10-10 15:39, John Ousterhout:
> > > ...
> > >
> > > Note: I see from the code that Linux and BSD set different default
> streams:
> > > Linux uses stdout, while BSD uses stderr. This patch retains the
> distinction,
> > > though I'm not sure why it is there.
> >
> > I don't know either.
> > What is best between stdout and stderr for logs?
> 
> I would guess that stdout makes more sense, since most log entries describe
> normal operation, not errors. I'm happy to make these consistent, but this
> would introduce a behavior change for BSD (which currently uses stderr);
> would that be considered antisocial?

No, that's OK to use stdout on BSD.

> > > -int
> > > -rte_eal_common_log_init(FILE *default_log)
> > > +void
> > > +rte_eal_log_set_default(FILE *default_log)
> > >  {
> > >       default_log_stream = default_log;
> > > -     rte_openlog_stream(default_log);
> > >
> > >  #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
> > >       RTE_LOG(NOTICE, EAL, "Debug logs available - lower
> performance\n");
> > >  #endif
> > > -
> > > -     return 0;
> > >  }
> >
> > Do we really need a function for that?
> > Why not just initialize default_log_stream statically?
> 
> Right now, different platforms have different defaults. BSD uses stderr
> always. Linux starts out with stdout as the default, but later during
> initialization it switches to a different default that logs messages both
> to  standard output and also to syslog. I don't have enough experience with
> DPDK to know whether a single approach is really right for all systems (or
> which approach it should be), and since I'm a DPDK newbie I thought it best
> to take a more conservative approach and avoid behavioral changes. My
> personal preference would be to minimize mission creep with this patch and
> leave that behavior in place for someone with more expertise to deal with
> later (and this issue is orthogonal to the main goal of the patch). But, if
> unifying the log defaults is considered essential to the patch (and is
> noncontroversial), I'm willing to implement it.

OK sorry, I'm mixing things.

1/ When removing early log functions, you are replacing early init with
a default set to stderr/stdout via rte_eal_log_set_default.
I think you can just set statically to stdout:
	static FILE *default_log_stream = stdout;

2/ Yes, on Linux, a more complex stream with stdout + syslog is set.
It is OK to use rte_eal_log_set_default for that usage.
Note that there is a stream which is not used and can be removed in eal_private.h:
	extern FILE *eal_default_log_stream;
Other note: rte_eal_log_set_default is not a public function so should be
named eal_log_set_default.

3/ When calling rte_eal_log_set_default on BSD from rte_eal_log_init,
you can keep stderr but an empty function would be better because
it is not called and already set (to stderr or stdout if 1/).

4/ rte_eal_log_init can be called earlier to replace early init.

5/ It would be simpler to understand by splitting in two patches
(remove early log + use non default log)

I understand that you prefer to focus on your fix and I'm more or less
suggesting a cleanup of logging. That's why I can do the first cleanup
patch if you are really not confortable with it. (I feel you could do it)
Just let me know.

Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

All of your suggestions look reasonable and fairly straightforward; I'll
work on a new patch that includes them.

Given that rte_eal_log_init is a no-op (and won't even be invoked), would
it be better to remove that function completely, and even delete the file
containing it (eal_log.c), or is it better to retain the empty function in
order to maintain a parallel structure with Linux? Personally I'd lean
towards deleting the file. As it stands, the interface to that function
doesn't even make sense for BSD; the arguments were chosen for Linux and
are ignored in BSD.

Let me know your preference.

-John-

On Tue, Oct 11, 2016 at 1:30 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2016-10-11 09:30, John Ousterhout:
> > On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon <
> thomas.monjalon@6wind.com>
> > wrote:
> > >
> > > 2016-10-10 15:39, John Ousterhout:
> > > > ...
> > > >
> > > > Note: I see from the code that Linux and BSD set different default
> > streams:
> > > > Linux uses stdout, while BSD uses stderr. This patch retains the
> > distinction,
> > > > though I'm not sure why it is there.
> > >
> > > I don't know either.
> > > What is best between stdout and stderr for logs?
> >
> > I would guess that stdout makes more sense, since most log entries
> describe
> > normal operation, not errors. I'm happy to make these consistent, but
> this
> > would introduce a behavior change for BSD (which currently uses stderr);
> > would that be considered antisocial?
>
> No, that's OK to use stdout on BSD.
>
> > > > -int
> > > > -rte_eal_common_log_init(FILE *default_log)
> > > > +void
> > > > +rte_eal_log_set_default(FILE *default_log)
> > > >  {
> > > >       default_log_stream = default_log;
> > > > -     rte_openlog_stream(default_log);
> > > >
> > > >  #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
> > > >       RTE_LOG(NOTICE, EAL, "Debug logs available - lower
> > performance\n");
> > > >  #endif
> > > > -
> > > > -     return 0;
> > > >  }
> > >
> > > Do we really need a function for that?
> > > Why not just initialize default_log_stream statically?
> >
> > Right now, different platforms have different defaults. BSD uses stderr
> > always. Linux starts out with stdout as the default, but later during
> > initialization it switches to a different default that logs messages both
> > to  standard output and also to syslog. I don't have enough experience
> with
> > DPDK to know whether a single approach is really right for all systems
> (or
> > which approach it should be), and since I'm a DPDK newbie I thought it
> best
> > to take a more conservative approach and avoid behavioral changes. My
> > personal preference would be to minimize mission creep with this patch
> and
> > leave that behavior in place for someone with more expertise to deal with
> > later (and this issue is orthogonal to the main goal of the patch). But,
> if
> > unifying the log defaults is considered essential to the patch (and is
> > noncontroversial), I'm willing to implement it.
>
> OK sorry, I'm mixing things.
>
> 1/ When removing early log functions, you are replacing early init with
> a default set to stderr/stdout via rte_eal_log_set_default.
> I think you can just set statically to stdout:
>         static FILE *default_log_stream = stdout;
>
> 2/ Yes, on Linux, a more complex stream with stdout + syslog is set.
> It is OK to use rte_eal_log_set_default for that usage.
> Note that there is a stream which is not used and can be removed in
> eal_private.h:
>         extern FILE *eal_default_log_stream;
> Other note: rte_eal_log_set_default is not a public function so should be
> named eal_log_set_default.
>
> 3/ When calling rte_eal_log_set_default on BSD from rte_eal_log_init,
> you can keep stderr but an empty function would be better because
> it is not called and already set (to stderr or stdout if 1/).
>
> 4/ rte_eal_log_init can be called earlier to replace early init.
>
> 5/ It would be simpler to understand by splitting in two patches
> (remove early log + use non default log)
>
> I understand that you prefer to focus on your fix and I'm more or less
> suggesting a cleanup of logging. That's why I can do the first cleanup
> patch if you are really not confortable with it. (I feel you could do it)
> Just let me know.
>

Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[Thomas Monjalon]

2016-10-11 14:46, John Ousterhout:
> All of your suggestions look reasonable and fairly straightforward; I'll
> work on a new patch that includes them.
> 
> Given that rte_eal_log_init is a no-op (and won't even be invoked), would
> it be better to remove that function completely, and even delete the file
> containing it (eal_log.c), or is it better to retain the empty function in
> order to maintain a parallel structure with Linux? Personally I'd lean
> towards deleting the file. As it stands, the interface to that function
> doesn't even make sense for BSD; the arguments were chosen for Linux and
> are ignored in BSD.
> 
> Let me know your preference.

Yes you can remove the file.

Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[Don Provan]

> -----Original Message-----
> From: John Ousterhout [mailto:ouster@cs.stanford.edu]
> Sent: Tuesday, October 11, 2016 9:30 AM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls
> before rte_eal_init
> 
> On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon
> <thomas.monjalon@6wind.com>
> wrote:
> > I don't know either.
> > What is best between stdout and stderr for logs?
> 
> I would guess that stdout makes more sense, since most log entries describe
> normal operation, not errors. I'm happy to make these consistent, but this
> would introduce a behavior change for BSD (which currently uses stderr);
> would that be considered antisocial?

I've never seen a pronouncement or anything, but as a linux programmer,
my attitude is that stdout should be the output the application is producing
when carrying out its function. Debugging output isn't part of what the
application is trying to accomplish, so it should be sent to stderr where it
can be segregated from the functional output when needed.
-don
dprovan@bivio.net

Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

Don's argument for stderr over stdout makes sense to me. Does anyone else
disagree?

-John-

On Tue, Oct 11, 2016 at 3:16 PM, Don Provan <dprovan@bivio.net> wrote:

> > -----Original Message-----
> > From: John Ousterhout [mailto:ouster@cs.stanford.edu]
> > Sent: Tuesday, October 11, 2016 9:30 AM
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls
> > before rte_eal_init
> >
> > On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon
> > <thomas.monjalon@6wind.com>
> > wrote:
> > > I don't know either.
> > > What is best between stdout and stderr for logs?
> >
> > I would guess that stdout makes more sense, since most log entries
> describe
> > normal operation, not errors. I'm happy to make these consistent, but
> this
> > would introduce a behavior change for BSD (which currently uses stderr);
> > would that be considered antisocial?
>
> I've never seen a pronouncement or anything, but as a linux programmer,
> my attitude is that stdout should be the output the application is
> producing
> when carrying out its function. Debugging output isn't part of what the
> application is trying to accomplish, so it should be sent to stderr where
> it
> can be segregated from the functional output when needed.
> -don
> dprovan@bivio.net
>
>

[dpdk-dev] [PATCH v3] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

Before this patch, application-specific loggers could not be
installed before rte_eal_init completed (the initialization process
called rte_openlog_stream, overwriting any previously installed
logger). This made it impossible for an application to capture the
initial log messages generated during rte_eal_init. This patch changes
initialization so that information from a previous call to
rte_openlog_stream is not lost. Specifically:
* The default log stream is now maintained separately from an
  application-specific log stream installed with rte_openlog_stream.
* rte_eal_common_log_init has been renamed to eal_log_set_default,
  since this is all it does. It no longer invokes rte_openlog_stream; it
  just updates the default stream. Also, this method now returns void,
  rather than int, since there are no errors.

This patch also removes the "early log" mechanism and cleans up the
log initialization mechanism:
* The default log stream defaults to stderr on all platforms if
  eal_log_set_default hasn't been invoked (Linux used to use stdout
  during the first part of initialization).
* Removed rte_eal_log_early_init; all of the desired functionality can
  be achieved by calling eal_log_set_default.
* Removed lib/librte_eal/bsdapp/eal/eal_log.c: it contained only one
  function, rte_eal_log_init, which is not needed or invoked for BSD.
* Removed declaration for eal_default_log_stream in rte_log.h (it's now
  private to eal_common_log.c).
* Moved call to rte_eal_log_init earlier in rte_eal_init for Linux, so
  that it starts using the preferrred log ASAP.

Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
----
v3:
* Made stderr the initial default log stream for Linux.
* Deleted lib/librte_eal/bsdapp/eal/eal_log.c.
* Deleted declaration for eal_default_log_stream in rte_log.h.
* Moved rte_eal_log_init call for Linux.

v2:
* Removed the early log mechanism, renamed rte_eal_common_log_init.
---
 lib/librte_eal/bsdapp/eal/Makefile      |  3 +-
 lib/librte_eal/bsdapp/eal/eal.c         |  6 ----
 lib/librte_eal/bsdapp/eal/eal_log.c     | 57 ---------------------------------
 lib/librte_eal/common/eal_common_log.c  | 29 ++++++++++-------
 lib/librte_eal/common/eal_private.h     | 16 +++------
 lib/librte_eal/common/include/rte_log.h |  5 +--
 lib/librte_eal/linuxapp/eal/eal.c       |  9 ++----
 lib/librte_eal/linuxapp/eal/eal_log.c   | 40 +----------------------
 8 files changed, 28 insertions(+), 137 deletions(-)
 delete mode 100644 lib/librte_eal/bsdapp/eal/eal_log.c

diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index 7a0fea5..34340c1 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -50,12 +50,11 @@ EXPORT_MAP := rte_eal_version.map
 
 LIBABIVER := 3
 
-# specific to linuxapp exec-env
+# specific to bsdapp exec-env
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) := eal.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_memory.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_hugepage_info.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_thread.c
-SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_log.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_pci.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_debug.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_lcore.c
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index a0c8f8c..6a6ae86 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -501,9 +501,6 @@ rte_eal_init(int argc, char **argv)
 
 	thread_id = pthread_self();
 
-	if (rte_eal_log_early_init() < 0)
-		rte_panic("Cannot init early logs\n");
-
 	eal_log_level_parse(argc, argv);
 
 	/* set log level as early as possible */
@@ -552,9 +549,6 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_tailqs_init() < 0)
 		rte_panic("Cannot init tail queues for objects\n");
 
-/*	if (rte_eal_log_init(argv[0], internal_config.syslog_facility) < 0)
-		rte_panic("Cannot init logs\n");*/
-
 	if (rte_eal_alarm_init() < 0)
 		rte_panic("Cannot init interrupt-handling thread\n");
 
diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c b/lib/librte_eal/bsdapp/eal/eal_log.c
deleted file mode 100644
index a425f7a..0000000
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/*-
- *   BSD LICENSE
- *
- *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
- *   All rights reserved.
- *
- *   Redistribution and use in source and binary forms, with or without
- *   modification, are permitted provided that the following conditions
- *   are met:
- *
- *     * Redistributions of source code must retain the above copyright
- *       notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above copyright
- *       notice, this list of conditions and the following disclaimer in
- *       the documentation and/or other materials provided with the
- *       distribution.
- *     * Neither the name of Intel Corporation nor the names of its
- *       contributors may be used to endorse or promote products derived
- *       from this software without specific prior written permission.
- *
- *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include <stdio.h>
-#include <rte_common.h>
-#include <rte_log.h>
-
-#include <eal_private.h>
-
-/*
- * set the log to default function, called during eal init process,
- * once memzones are available.
- */
-int
-rte_eal_log_init(const char *id __rte_unused, int facility __rte_unused)
-{
-	if (rte_eal_common_log_init(stderr) < 0)
-		return -1;
-	return 0;
-}
-
-int
-rte_eal_log_early_init(void)
-{
-	rte_openlog_stream(stderr);
-	return 0;
-}
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 967991a..3e7a386 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -48,11 +48,12 @@ struct rte_logs rte_logs = {
 	.file = NULL,
 };
 
+/* Stream to use for logging if rte_logs.file is NULL */
 static FILE *default_log_stream;
 
 /**
  * This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
  */
 struct log_cur_msg {
 	uint32_t loglevel; /**< log level - see rte_log.h */
@@ -68,10 +69,7 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
 int
 rte_openlog_stream(FILE *f)
 {
-	if (f == NULL)
-		rte_logs.file = default_log_stream;
-	else
-		rte_logs.file = f;
+	rte_logs.file = f;
 	return 0;
 }
 
@@ -127,6 +125,18 @@ rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
 {
 	int ret;
 	FILE *f = rte_logs.file;
+	if (f == NULL) {
+		f = default_log_stream;
+		if (f == NULL) {
+			/*
+			 * Grab the current value of stderr here, rather than just
+			 * initializing default_log_stream to stderr. This ensures
+			 * that we will always use the current value of stderr,
+			 * even if the application closes and reopens it. 
+			 */
+			f = stderr;
+		}
+	}
 
 	if ((level > rte_logs.level) || !(logtype & rte_logs.type))
 		return 0;
@@ -158,17 +168,14 @@ rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
 }
 
 /*
- * called by environment-specific log init function
+ * Called by environment-specific initialization functions.
  */
-int
-rte_eal_common_log_init(FILE *default_log)
+void
+eal_log_set_default(FILE *default_log)
 {
 	default_log_stream = default_log;
-	rte_openlog_stream(default_log);
 
 #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
 	RTE_LOG(NOTICE, EAL, "Debug logs available - lower performance\n");
 #endif
-
-	return 0;
 }
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 19f7535..0ab9eba 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -47,7 +47,9 @@
 int rte_eal_memzone_init(void);
 
 /**
- * Common log initialization function (private to eal).
+ * Common log initialization function (private to eal).  Determines
+ * where log data is written when no call to rte_openlog_stream is
+ * in effect.
  *
  * @param default_log
  *   The default log stream to be used.
@@ -55,7 +57,7 @@ int rte_eal_memzone_init(void);
  *   - 0 on success
  *   - Negative on error
  */
-int rte_eal_common_log_init(FILE *default_log);
+void eal_log_set_default(FILE *default_log);
 
 /**
  * Fill configuration with number of physical and logical processors
@@ -97,16 +99,6 @@ int rte_eal_memory_init(void);
 int rte_eal_timer_init(void);
 
 /**
- * Init early logs
- *
- * This function is private to EAL.
- *
- * @return
- *   0 on success, negative on error
- */
-int rte_eal_log_early_init(void);
-
-/**
  * Init the default log stream
  *
  * This function is private to EAL.
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 919563c..29f7d19 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -54,7 +54,7 @@ extern "C" {
 struct rte_logs {
 	uint32_t type;  /**< Bitfield with enabled logs. */
 	uint32_t level; /**< Log level. */
-	FILE *file;     /**< Pointer to current FILE* for logs. */
+	FILE *file;     /**< Output file set by rte_openlog_stream, or NULL. */
 };
 
 /** Global log informations */
@@ -100,9 +100,6 @@ extern struct rte_logs rte_logs;
 #define RTE_LOG_INFO     7U  /**< Informational.                    */
 #define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
 
-/** The default log stream. */
-extern FILE *eal_default_log_stream;
-
 /**
  * Change the stream that will be used by the logging system.
  *
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index d5b81a3..1c7bacc 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -748,9 +748,6 @@ rte_eal_init(int argc, char **argv)
 
 	thread_id = pthread_self();
 
-	if (rte_eal_log_early_init() < 0)
-		rte_panic("Cannot init early logs\n");
-
 	eal_log_level_parse(argc, argv);
 
 	/* set log level as early as possible */
@@ -789,6 +786,9 @@ rte_eal_init(int argc, char **argv)
 
 	rte_config_init();
 
+	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0)
+		rte_panic("Cannot init logs\n");
+
 	if (rte_eal_pci_init() < 0)
 		rte_panic("Cannot init PCI\n");
 
@@ -809,9 +809,6 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_tailqs_init() < 0)
 		rte_panic("Cannot init tail queues for objects\n");
 
-	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0)
-		rte_panic("Cannot init logs\n");
-
 	if (rte_eal_alarm_init() < 0)
 		rte_panic("Cannot init interrupt-handling thread\n");
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index d391100..e3a50aa 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -97,45 +97,7 @@ rte_eal_log_init(const char *id, int facility)
 
 	openlog(id, LOG_NDELAY | LOG_PID, facility);
 
-	if (rte_eal_common_log_init(log_stream) < 0)
-		return -1;
-
-	return 0;
-}
-
-/* early logs */
-
-/*
- * early log function, used before rte_eal_log_init
- */
-static ssize_t
-early_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
-{
-	ssize_t ret;
-	ret = fwrite(buf, size, 1, stdout);
-	fflush(stdout);
-	if (ret == 0)
-		return -1;
-	return ret;
-}
-
-static cookie_io_functions_t early_log_func = {
-	.write = early_log_write,
-};
-static FILE *early_log_stream;
+	eal_log_set_default(log_stream);
 
-/*
- * init the log library, called by rte_eal_init() to enable early
- * logs
- */
-int
-rte_eal_log_early_init(void)
-{
-	early_log_stream = fopencookie(NULL, "w+", early_log_func);
-	if (early_log_stream == NULL) {
-		printf("Cannot configure early_log_stream\n");
-		return -1;
-	}
-	rte_openlog_stream(early_log_stream);
 	return 0;
 }
-- 
2.8.3

[dpdk-dev] [PATCH v4] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

Before this patch, application-specific loggers could not be
installed before rte_eal_init completed (the initialization process
called rte_openlog_stream, overwriting any previously installed
logger). This made it impossible for an application to capture the
initial log messages generated during rte_eal_init. This patch changes
initialization so that information from a previous call to
rte_openlog_stream is not lost. Specifically:
* The default log stream is now maintained separately from an
  application-specific log stream installed with rte_openlog_stream.
* rte_eal_common_log_init has been renamed to eal_log_set_default,
  since this is all it does. It no longer invokes rte_openlog_stream; it
  just updates the default stream. Also, this method now returns void,
  rather than int, since there are no errors.

This patch also removes the "early log" mechanism and cleans up the
log initialization mechanism:
* The default log stream defaults to stderr on all platforms if
  eal_log_set_default hasn't been invoked (Linux used to use stdout
  during the first part of initialization).
* Removed rte_eal_log_early_init; all of the desired functionality can
  be achieved by calling eal_log_set_default.
* Removed lib/librte_eal/bsdapp/eal/eal_log.c: it contained only one
  function, rte_eal_log_init, which is not needed or invoked for BSD.
* Removed declaration for eal_default_log_stream in rte_log.h (it's now
  private to eal_common_log.c).
* Moved call to rte_eal_log_init earlier in rte_eal_init for Linux, so
  that it starts using the preferrred log ASAP.

Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
----
v4:
* Fixed problems from checkpatches.

v3:
* Made stderr the initial default log stream for Linux.
* Deleted lib/librte_eal/bsdapp/eal/eal_log.c.
* Deleted declaration for eal_default_log_stream in rte_log.h.
* Moved rte_eal_log_init call for Linux.

v2:
* Removed the early log mechanism, renamed rte_eal_common_log_init.
---
 lib/librte_eal/bsdapp/eal/Makefile      |  3 +-
 lib/librte_eal/bsdapp/eal/eal.c         |  6 ----
 lib/librte_eal/bsdapp/eal/eal_log.c     | 57 ---------------------------------
 lib/librte_eal/common/eal_common_log.c  | 30 ++++++++++-------
 lib/librte_eal/common/eal_private.h     | 16 +++------
 lib/librte_eal/common/include/rte_log.h |  5 +--
 lib/librte_eal/linuxapp/eal/eal.c       |  9 ++----
 lib/librte_eal/linuxapp/eal/eal_log.c   | 40 +----------------------
 8 files changed, 29 insertions(+), 137 deletions(-)
 delete mode 100644 lib/librte_eal/bsdapp/eal/eal_log.c

diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index 7a0fea5..34340c1 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -50,12 +50,11 @@ EXPORT_MAP := rte_eal_version.map
 
 LIBABIVER := 3
 
-# specific to linuxapp exec-env
+# specific to bsdapp exec-env
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) := eal.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_memory.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_hugepage_info.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_thread.c
-SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_log.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_pci.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_debug.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_lcore.c
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index a0c8f8c..6a6ae86 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -501,9 +501,6 @@ rte_eal_init(int argc, char **argv)
 
 	thread_id = pthread_self();
 
-	if (rte_eal_log_early_init() < 0)
-		rte_panic("Cannot init early logs\n");
-
 	eal_log_level_parse(argc, argv);
 
 	/* set log level as early as possible */
@@ -552,9 +549,6 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_tailqs_init() < 0)
 		rte_panic("Cannot init tail queues for objects\n");
 
-/*	if (rte_eal_log_init(argv[0], internal_config.syslog_facility) < 0)
-		rte_panic("Cannot init logs\n");*/
-
 	if (rte_eal_alarm_init() < 0)
 		rte_panic("Cannot init interrupt-handling thread\n");
 
diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c b/lib/librte_eal/bsdapp/eal/eal_log.c
deleted file mode 100644
index a425f7a..0000000
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/*-
- *   BSD LICENSE
- *
- *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
- *   All rights reserved.
- *
- *   Redistribution and use in source and binary forms, with or without
- *   modification, are permitted provided that the following conditions
- *   are met:
- *
- *     * Redistributions of source code must retain the above copyright
- *       notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above copyright
- *       notice, this list of conditions and the following disclaimer in
- *       the documentation and/or other materials provided with the
- *       distribution.
- *     * Neither the name of Intel Corporation nor the names of its
- *       contributors may be used to endorse or promote products derived
- *       from this software without specific prior written permission.
- *
- *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include <stdio.h>
-#include <rte_common.h>
-#include <rte_log.h>
-
-#include <eal_private.h>
-
-/*
- * set the log to default function, called during eal init process,
- * once memzones are available.
- */
-int
-rte_eal_log_init(const char *id __rte_unused, int facility __rte_unused)
-{
-	if (rte_eal_common_log_init(stderr) < 0)
-		return -1;
-	return 0;
-}
-
-int
-rte_eal_log_early_init(void)
-{
-	rte_openlog_stream(stderr);
-	return 0;
-}
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 967991a..e45d326 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -48,11 +48,12 @@ struct rte_logs rte_logs = {
 	.file = NULL,
 };
 
+/* Stream to use for logging if rte_logs.file is NULL */
 static FILE *default_log_stream;
 
 /**
  * This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
  */
 struct log_cur_msg {
 	uint32_t loglevel; /**< log level - see rte_log.h */
@@ -68,10 +69,7 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
 int
 rte_openlog_stream(FILE *f)
 {
-	if (f == NULL)
-		rte_logs.file = default_log_stream;
-	else
-		rte_logs.file = f;
+	rte_logs.file = f;
 	return 0;
 }
 
@@ -127,6 +125,19 @@ rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
 {
 	int ret;
 	FILE *f = rte_logs.file;
+	if (f == NULL) {
+		f = default_log_stream;
+		if (f == NULL) {
+			/*
+			 * Grab the current value of stderr here, rather than
+			 * just initializing default_log_stream to stderr. This
+			 * ensures that we will always use the current value
+			 * of stderr, even if the application closes and
+			 * reopens it.
+			 */
+			f = stderr;
+		}
+	}
 
 	if ((level > rte_logs.level) || !(logtype & rte_logs.type))
 		return 0;
@@ -158,17 +169,14 @@ rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
 }
 
 /*
- * called by environment-specific log init function
+ * Called by environment-specific initialization functions.
  */
-int
-rte_eal_common_log_init(FILE *default_log)
+void
+eal_log_set_default(FILE *default_log)
 {
 	default_log_stream = default_log;
-	rte_openlog_stream(default_log);
 
 #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
 	RTE_LOG(NOTICE, EAL, "Debug logs available - lower performance\n");
 #endif
-
-	return 0;
 }
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 19f7535..0ab9eba 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -47,7 +47,9 @@
 int rte_eal_memzone_init(void);
 
 /**
- * Common log initialization function (private to eal).
+ * Common log initialization function (private to eal).  Determines
+ * where log data is written when no call to rte_openlog_stream is
+ * in effect.
  *
  * @param default_log
  *   The default log stream to be used.
@@ -55,7 +57,7 @@ int rte_eal_memzone_init(void);
  *   - 0 on success
  *   - Negative on error
  */
-int rte_eal_common_log_init(FILE *default_log);
+void eal_log_set_default(FILE *default_log);
 
 /**
  * Fill configuration with number of physical and logical processors
@@ -97,16 +99,6 @@ int rte_eal_memory_init(void);
 int rte_eal_timer_init(void);
 
 /**
- * Init early logs
- *
- * This function is private to EAL.
- *
- * @return
- *   0 on success, negative on error
- */
-int rte_eal_log_early_init(void);
-
-/**
  * Init the default log stream
  *
  * This function is private to EAL.
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 919563c..29f7d19 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -54,7 +54,7 @@ extern "C" {
 struct rte_logs {
 	uint32_t type;  /**< Bitfield with enabled logs. */
 	uint32_t level; /**< Log level. */
-	FILE *file;     /**< Pointer to current FILE* for logs. */
+	FILE *file;     /**< Output file set by rte_openlog_stream, or NULL. */
 };
 
 /** Global log informations */
@@ -100,9 +100,6 @@ extern struct rte_logs rte_logs;
 #define RTE_LOG_INFO     7U  /**< Informational.                    */
 #define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
 
-/** The default log stream. */
-extern FILE *eal_default_log_stream;
-
 /**
  * Change the stream that will be used by the logging system.
  *
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index d5b81a3..1c7bacc 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -748,9 +748,6 @@ rte_eal_init(int argc, char **argv)
 
 	thread_id = pthread_self();
 
-	if (rte_eal_log_early_init() < 0)
-		rte_panic("Cannot init early logs\n");
-
 	eal_log_level_parse(argc, argv);
 
 	/* set log level as early as possible */
@@ -789,6 +786,9 @@ rte_eal_init(int argc, char **argv)
 
 	rte_config_init();
 
+	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0)
+		rte_panic("Cannot init logs\n");
+
 	if (rte_eal_pci_init() < 0)
 		rte_panic("Cannot init PCI\n");
 
@@ -809,9 +809,6 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_tailqs_init() < 0)
 		rte_panic("Cannot init tail queues for objects\n");
 
-	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0)
-		rte_panic("Cannot init logs\n");
-
 	if (rte_eal_alarm_init() < 0)
 		rte_panic("Cannot init interrupt-handling thread\n");
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index d391100..e3a50aa 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -97,45 +97,7 @@ rte_eal_log_init(const char *id, int facility)
 
 	openlog(id, LOG_NDELAY | LOG_PID, facility);
 
-	if (rte_eal_common_log_init(log_stream) < 0)
-		return -1;
-
-	return 0;
-}
-
-/* early logs */
-
-/*
- * early log function, used before rte_eal_log_init
- */
-static ssize_t
-early_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
-{
-	ssize_t ret;
-	ret = fwrite(buf, size, 1, stdout);
-	fflush(stdout);
-	if (ret == 0)
-		return -1;
-	return ret;
-}
-
-static cookie_io_functions_t early_log_func = {
-	.write = early_log_write,
-};
-static FILE *early_log_stream;
+	eal_log_set_default(log_stream);
 
-/*
- * init the log library, called by rte_eal_init() to enable early
- * logs
- */
-int
-rte_eal_log_early_init(void)
-{
-	early_log_stream = fopencookie(NULL, "w+", early_log_func);
-	if (early_log_stream == NULL) {
-		printf("Cannot configure early_log_stream\n");
-		return -1;
-	}
-	rte_openlog_stream(early_log_stream);
 	return 0;
 }
-- 
2.8.3

Re: [dpdk-dev] [PATCH v4] log: respect rte_openlog_stream calls before rte_eal_init

[Thomas Monjalon]

2016-10-12 12:38, John Ousterhout:
> @@ -127,6 +125,19 @@ rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
>  {
>  	int ret;
>  	FILE *f = rte_logs.file;
> +	if (f == NULL) {
> +		f = default_log_stream;
> +		if (f == NULL) {
> +			/*
> +			 * Grab the current value of stderr here, rather than
> +			 * just initializing default_log_stream to stderr. This
> +			 * ensures that we will always use the current value
> +			 * of stderr, even if the application closes and
> +			 * reopens it.
> +			 */
> +			f = stderr;
> +		}
> +	}

I don't understand this big comment.
What is the difference with initializing default_log_stream to stderr?
What do you mean by "if the application closes and reopens it"?

Re: [dpdk-dev] [PATCH v4] log: respect rte_openlog_stream calls before rte_eal_init

[John Ousterhout]

Suppose an application starts up, calls rte_eal_init, then later on invokes
code like this:

fclose(stderr);
stderr = fopen("foo", "w");

This might happen if it is using stderr for its log, but decides to roll
the log over to a new file.

Now stderr has changed.  However, if DPDK made a copy of it with a
statement like this:

FILE *default_log_stream = stderr;

then default_log_stream will continue to refer to the old log file, not the
new one.

Thus, it's better to grab the value of stderr at the last possible moment
before logging.

-John-

On Wed, Oct 12, 2016 at 12:47 PM, Thomas Monjalon <thomas.monjalon@6wind.com
> wrote:

> 2016-10-12 12:38, John Ousterhout:
> > @@ -127,6 +125,19 @@ rte_vlog(uint32_t level, uint32_t logtype, const
> char *format, va_list ap)
> >  {
> >       int ret;
> >       FILE *f = rte_logs.file;
> > +     if (f == NULL) {
> > +             f = default_log_stream;
> > +             if (f == NULL) {
> > +                     /*
> > +                      * Grab the current value of stderr here, rather
> than
> > +                      * just initializing default_log_stream to stderr.
> This
> > +                      * ensures that we will always use the current
> value
> > +                      * of stderr, even if the application closes and
> > +                      * reopens it.
> > +                      */
> > +                     f = stderr;
> > +             }
> > +     }
>
> I don't understand this big comment.
> What is the difference with initializing default_log_stream to stderr?
> What do you mean by "if the application closes and reopens it"?
>

Re: [dpdk-dev] [PATCH v4] log: respect rte_openlog_stream calls before rte_eal_init

[Thomas Monjalon]

2016-10-12 12:38, John Ousterhout:
> Before this patch, application-specific loggers could not be
> installed before rte_eal_init completed (the initialization process
> called rte_openlog_stream, overwriting any previously installed
> logger). This made it impossible for an application to capture the
> initial log messages generated during rte_eal_init. This patch changes
> initialization so that information from a previous call to
> rte_openlog_stream is not lost. Specifically:
> * The default log stream is now maintained separately from an
>   application-specific log stream installed with rte_openlog_stream.
> * rte_eal_common_log_init has been renamed to eal_log_set_default,
>   since this is all it does. It no longer invokes rte_openlog_stream; it
>   just updates the default stream. Also, this method now returns void,
>   rather than int, since there are no errors.
> 
> This patch also removes the "early log" mechanism and cleans up the
> log initialization mechanism:
> * The default log stream defaults to stderr on all platforms if
>   eal_log_set_default hasn't been invoked (Linux used to use stdout
>   during the first part of initialization).
> * Removed rte_eal_log_early_init; all of the desired functionality can
>   be achieved by calling eal_log_set_default.
> * Removed lib/librte_eal/bsdapp/eal/eal_log.c: it contained only one
>   function, rte_eal_log_init, which is not needed or invoked for BSD.
> * Removed declaration for eal_default_log_stream in rte_log.h (it's now
>   private to eal_common_log.c).
> * Moved call to rte_eal_log_init earlier in rte_eal_init for Linux, so
>   that it starts using the preferrred log ASAP.
> 
> Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>

Applied, thanks