[i-g-t,3/5] lib: introduce log domains

Commit Message

Thomas Wood Dec. 2, 2014, 5:15 p.m. UTC

Log domains can be used to identify the source of log messages, such as
the test being run or the helper library.

Signed-off-by: Thomas Wood <[e-mail]>
---
 lib/Makefile.am |  3 ++-
 lib/igt_core.c  | 21 +++++++++++++++------
 lib/igt_core.h  | 18 +++++++++++-------
 lib/igt_kms.c   |  2 +-
 4 files changed, 29 insertions(+), 15 deletions(-)

Comments

Daniel Vetter Dec. 3, 2014, 2:22 p.m. UTC | #1

On Tue, Dec 02, 2014 at 05:15:37PM +0000, Thomas Wood wrote:
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index a258348..5c5ee25 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -512,16 +512,20 @@ bool igt_run_in_simulation(void);
>  void igt_skip_on_simulation(void);
>  
>  /* structured logging */
> +#ifndef IGT_LOG_DOMAIN
> +#define IGT_LOG_DOMAIN (NULL)
> +#endif

I guess the idea is that different source files have a

#define IGT_LOG_DOMAIN "foobar"

at the top before any includes?

Would be great if you can add some good defaults as a patch on top:
- For everything in tests/*c I think we should add a suitable define in
  the Makefile, perhaps "test"
- Same for tools/ with "tools"
- lib/* should probably have per-file log domains, using the basename of
  the source file itself. Dunno whether there's a cool Makefile trick to
  make that happen.

With that added I think patches 1-4 are a nice addition to the igt
logging.
-Daniel

Thomas Wood Dec. 4, 2014, 10:58 a.m. UTC | #2

On 3 December 2014 at 14:22, Daniel Vetter <[e-mail]> wrote:

> On Tue, Dec 02, 2014 at 05:15:37PM +0000, Thomas Wood wrote:
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index a258348..5c5ee25 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -512,16 +512,20 @@ bool igt_run_in_simulation(void);
> >  void igt_skip_on_simulation(void);
> >
> >  /* structured logging */
> > +#ifndef IGT_LOG_DOMAIN
> > +#define IGT_LOG_DOMAIN (NULL)
> > +#endif
>
> I guess the idea is that different source files have a
>
> #define IGT_LOG_DOMAIN "foobar"
>
> at the top before any includes?
>

Yes, or defined with -D in CFLAGS.


>
> Would be great if you can add some good defaults as a patch on top:
> - For everything in tests/*c I think we should add a suitable define in
>   the Makefile, perhaps "test"
>

The default domain for a test is the program name.



> - Same for tools/ with "tools"
> - lib/* should probably have per-file log domains, using the basename of
>   the source file itself. Dunno whether there's a cool Makefile trick to
>   make that happen.
>

 The helper library is currently all within the same domain (defined in
Makefile.am), but separate domains for different parts of the library would
improve the filtering options.



>
> With that added I think patches 1-4 are a nice addition to the igt
> logging.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch
>

Patch

diff --git a/lib/Makefile.am b/lib/Makefile.am
index ab82302..b0aeef3 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -10,7 +10,8 @@  noinst_HEADERS = check-ndebug.h
 
 AM_CPPFLAGS = -I$(top_srcdir)
 AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS)  \
-	    -DIGT_DATADIR=\""$(abs_top_srcdir)/tests"\"
+	    -DIGT_DATADIR=\""$(abs_top_srcdir)/tests"\" \
+	    -DIGT_LOG_DOMAIN=\""libintel_tools"\"
 
 
 LDADD = $(CAIRO_LIBS)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index f7763b5..4f4cf96 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1429,12 +1429,12 @@  void igt_skip_on_simulation(void)
  * are disabled. "none" completely disables all output and is not recommended
  * since crucial issues only reported at the IGT_LOG_WARN level are ignored.
  */
-void igt_log(enum igt_log_level level, const char *format, ...)
+void igt_log(const char *domain, enum igt_log_level level, const char *format, ...)
 {
 	va_list args;
 
 	va_start(args, format);
-	igt_vlog(level, format, args)
+	igt_vlog(domain, level, format, args);
 	va_end(args);
 }
 
@@ -1451,8 +1451,10 @@  void igt_log(enum igt_log_level level, const char *format, ...)
  * If there is no need to wrap up a vararg list in the caller it is simpler to
  * just use igt_log().
  */
-void igt_vlog(enum igt_log_level level, const char *format, va_list args)
+void igt_vlog(const char *domain, enum igt_log_level level, const char *format, va_list args)
 {
+	FILE *file;
+
 	assert(format);
 
 	if (list_subtests)
@@ -1461,11 +1463,18 @@  void igt_vlog(enum igt_log_level level, const char *format, va_list args)
 	if (igt_log_level > level)
 		return;
 
+	if (!domain)
+		domain = command_str;
+
 	if (level == IGT_LOG_WARN) {
+		file = stderr;
 		fflush(stdout);
-		vfprintf(stderr, format, args);
-	} else
-		vprintf(format, args);
+	}
+	else
+		file = stdout;
+
+	fprintf(file, "(%s:%d) ", domain, getpid());
+	vfprintf(file, format, args);
 }
 
 static void igt_alarm_handler(int signal)
diff --git a/lib/igt_core.h b/lib/igt_core.h
index a258348..5c5ee25 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -512,16 +512,20 @@  bool igt_run_in_simulation(void);
 void igt_skip_on_simulation(void);
 
 /* structured logging */
+#ifndef IGT_LOG_DOMAIN
+#define IGT_LOG_DOMAIN (NULL)
+#endif
+
 enum igt_log_level {
 	IGT_LOG_DEBUG,
 	IGT_LOG_INFO,
 	IGT_LOG_WARN,
 	IGT_LOG_NONE,
 };
-__attribute__((format(printf, 2, 3)))
-void igt_log(enum igt_log_level level, const char *format, ...);
-__attribute__((format(printf, 2, 0)))
-void igt_vlog(enum igt_log_level level, const char *format, va_list args);
+__attribute__((format(printf, 3, 4)))
+void igt_log(const char *domain, enum igt_log_level level, const char *format, ...);
+__attribute__((format(printf, 3, 0)))
+void igt_vlog(const char *domain, enum igt_log_level level, const char *format, va_list args);
 
 /**
  * igt_debug:
@@ -529,7 +533,7 @@  void igt_vlog(enum igt_log_level level, const char *format, va_list args);
  *
  * Wrapper for igt_log() for message at the IGT_LOG_DEBUG level.
  */
-#define igt_debug(f...) igt_log(IGT_LOG_DEBUG, f)
+#define igt_debug(f...) igt_log(IGT_LOG_DOMAIN, IGT_LOG_DEBUG, f)
 
 /**
  * igt_info:
@@ -537,7 +541,7 @@  void igt_vlog(enum igt_log_level level, const char *format, va_list args);
  *
  * Wrapper for igt_log() for message at the IGT_LOG_INFO level.
  */
-#define igt_info(f...) igt_log(IGT_LOG_INFO, f)
+#define igt_info(f...) igt_log(IGT_LOG_DOMAIN, IGT_LOG_INFO, f)
 
 /**
  * igt_warn:
@@ -545,7 +549,7 @@  void igt_vlog(enum igt_log_level level, const char *format, va_list args);
  *
  * Wrapper for igt_log() for message at the IGT_LOG_WARN level.
  */
-#define igt_warn(f...) igt_log(IGT_LOG_WARN, f)
+#define igt_warn(f...) igt_log(IGT_LOG_DOMAIN, IGT_LOG_WARN, f)
 extern enum igt_log_level igt_log_level;
 
 /**
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index eb8e085..0421ee0 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -740,7 +740,7 @@  igt_display_log(igt_display_t *display, const char *fmt, ...)
 	igt_debug("display: ");
 	for (i = 0; i < display->log_shift; i++)
 		igt_debug("%s", LOG_SPACES);
-	igt_vlog(IGT_LOG_DEBUG, fmt, args);
+	igt_vlog(IGT_LOG_DOMAIN, IGT_LOG_DEBUG, fmt, args);
 	va_end(args);
 }