[PATCH] libtracefs: Implement tracefs_warning()

[PATCH] libtracefs: Implement tracefs_warning()

[Tzvetomir Stoyanov (VMware)]

The warning() function is used in a lot of places in the libracefs
library, and it is implemented as a dummy weak function. There is a
weak implementation of the same function in traceevent library, which
could cause a problem when both are used in an application.
Replaced warning() with tracefs_warning() and implemented it as a
weak function, specific to this library.
Added a __weak define in the library internal header file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs-local.h |  3 ++-
 src/tracefs-events.c    |  2 +-
 src/tracefs-instance.c  |  8 ++++----
 src/tracefs-utils.c     | 31 ++++++++++++++++++++++++++-----
 4 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/include/tracefs-local.h b/include/tracefs-local.h
index 73ec113..5a69ef7 100644
--- a/include/tracefs-local.h
+++ b/include/tracefs-local.h
@@ -7,6 +7,7 @@
 #define _TRACE_FS_LOCAL_H
 
 #define __hidden __attribute__((visibility ("hidden")))
+#define __weak __attribute__((weak))
 
 #define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0]))
 
@@ -22,7 +23,7 @@ struct tracefs_instance {
 };
 
 /* Can be overridden */
-void warning(const char *fmt, ...);
+void tracefs_warning(const char *fmt, ...);
 
 int str_read_file(const char *file, char **buffer);
 char *trace_append_file(const char *dir, const char *name);
diff --git a/src/tracefs-events.c b/src/tracefs-events.c
index da56943..9a706ab 100644
--- a/src/tracefs-events.c
+++ b/src/tracefs-events.c
@@ -43,7 +43,7 @@ page_to_kbuf(struct tep_handle *tep, void *page, int size)
 
 	kbuffer_load_subbuffer(kbuf, page);
 	if (kbuffer_subbuffer_size(kbuf) > size) {
-		warning("%s: page_size > size", __func__);
+		tracefs_warning("%s: page_size > size", __func__);
 		kbuffer_free(kbuf);
 		kbuf = NULL;
 	}
diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c
index bf2fabf..e87b360 100644
--- a/src/tracefs-instance.c
+++ b/src/tracefs-instance.c
@@ -203,7 +203,7 @@ int tracefs_instance_destroy(struct tracefs_instance *instance)
 	int ret = -1;
 
 	if (!instance || !instance->name) {
-		warning("Cannot remove top instance");
+		tracefs_warning("Cannot remove top instance");
 		return -1;
 	}
 
@@ -265,8 +265,8 @@ char *tracefs_instance_get_dir(struct tracefs_instance *instance)
 
 	ret = asprintf(&path, "%s/instances/%s", instance->trace_dir, instance->name);
 	if (ret < 0) {
-		warning("Failed to allocate path for instance %s",
-			 instance->name);
+		tracefs_warning("Failed to allocate path for instance %s",
+				instance->name);
 		return NULL;
 	}
 
@@ -308,7 +308,7 @@ static int write_file(const char *file, const char *str)
 
 	fd = open(file, O_WRONLY | O_TRUNC);
 	if (fd < 0) {
-		warning("Failed to open '%s'", file);
+		tracefs_warning("Failed to open '%s'", file);
 		return -1;
 	}
 	ret = write(fd, str, strlen(str));
diff --git a/src/tracefs-utils.c b/src/tracefs-utils.c
index 0e96f8e..9a70175 100644
--- a/src/tracefs-utils.c
+++ b/src/tracefs-utils.c
@@ -13,6 +13,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <errno.h>
 
 #include "tracefs.h"
 #include "tracefs-local.h"
@@ -23,8 +24,28 @@
 #define _STR(x) #x
 #define STR(x) _STR(x)
 
-void __attribute__((weak)) warning(const char *fmt, ...)
+static int __vlib_warning(const char *fmt, va_list ap)
 {
+	int ret = errno;
+
+	if (errno)
+		perror("libtracefs");
+
+	fprintf(stderr, "  ");
+	vfprintf(stderr, fmt, ap);
+
+	fprintf(stderr, "\n");
+
+	return ret;
+}
+
+void __weak tracefs_warning(const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	__vlib_warning(fmt, ap);
+	va_end(ap);
 }
 
 static int mount_tracefs(void)
@@ -76,7 +97,7 @@ __hidden char *trace_find_tracing_dir(void)
 
 	fp = fopen("/proc/mounts", "r");
 	if (!fp) {
-		warning("Can't open /proc/mounts for read");
+		tracefs_warning("Can't open /proc/mounts for read");
 		return NULL;
 	}
 
@@ -103,7 +124,7 @@ __hidden char *trace_find_tracing_dir(void)
 				fspath[PATH_MAX] = 0;
 			} else {
 				if (mount_debugfs() < 0) {
-					warning("debugfs not mounted, please mount");
+					tracefs_warning("debugfs not mounted, please mount");
 					free(debug_str);
 					return NULL;
 				}
@@ -200,7 +221,7 @@ __hidden int str_read_file(const char *file, char **buffer)
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0) {
-		warning("File %s not found", file);
+		tracefs_warning("File %s not found", file);
 		return -1;
 	}
 
@@ -210,7 +231,7 @@ __hidden int str_read_file(const char *file, char **buffer)
 			continue;
 		nbuf = realloc(buf, size+r+1);
 		if (!nbuf) {
-			warning("Failed to allocate file buffer");
+			tracefs_warning("Failed to allocate file buffer");
 			size = -1;
 			break;
 		}
-- 
2.30.2

Re: [PATCH] libtracefs: Implement tracefs_warning()

[Steven Rostedt]

On Wed,  7 Apr 2021 08:11:54 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> The warning() function is used in a lot of places in the libracefs
> library, and it is implemented as a dummy weak function. There is a
> weak implementation of the same function in traceevent library, which
> could cause a problem when both are used in an application.
> Replaced warning() with tracefs_warning() and implemented it as a
> weak function, specific to this library.
> Added a __weak define in the library internal header file.

I've been thinking about this more, and I think we only want one function
to override, and that would be:

void __weak tep_vwarning(const char *name, const char *fmt, va_list ap)
{
	if (errno)
		perror(name);
	
	fprintf(stderr, "  ");
	vfprintf(stderr, fmt, ap);
	fprintf(stderr, "\n");
}


And simply have:

void tep_warning(const char *fmt, ...)
{
	va_list ap;

	va_start(ap, fmt);
	tep_vwarning("libtraceevent", fmt, ap);
	va_end(ap)
}

and

void tracefs_warning(const char *fmt, ...)
{
	va_list ap;

	va_start(ap, fmt);
	tep_vwarning("libtracefs", fmt, ap);
	va_end(ap)
}

and

void tracecmd_warning(const char *fmt, ...)
{
	va_list ap;

	va_start(ap, fmt);
	tep_vwarning("libtracecmd", fmt, ap);
	va_end(ap)
}

Then the application only needs to overwrite one function if they want
full control of the output. And it can use the name field to know if it
wants to do something different with different libraries.

Thus, trace-cmd could do:

void tep_vwarning(const char *name, const char *fmt, va_list ap)
{
	if (errno)
		perror("trace-cmd");

	fprintf(stderr, "  ");
	vfprintf(stderr, fmt, ap);
	fprintf(stderr, "\n");
}

and this will make all the warning messages from all the libraries print
the error from "trace-cmd" (we want the tool not the library), just like it
does today.

We could also have kernelshark have:

void tep_vwarning(const char *name, const char *fmt, va_list ap)
{
	char buf[BUFSIZ + 1];
	char *p = &buf[0];
	int r, left = BUFSIZ;

	if (errno) {
		r = snprintf(p, left, "kernelshark: %s\n", strerror(errno));
		left -= r;
		p += r;
	}

	if (left > 0) {
		r = snprintf(p, left, " ");
		p += r;
		left -= r;
	}

	if (left > 0) {
		r = vsnprintf(p, left, fmt, ap);
		left -= r;
		p += r;
	}

	if (left > 0)
		snprintf(p, left, "\n");

	gui_popup(buf);
}

Doing the above with a single function tep_vprintf() means that the
application doesn't need to define multiple functions.

-- Steve

Re: [PATCH] libtracefs: Implement tracefs_warning()

[Tzvetomir Stoyanov]

On Wed, Apr 7, 2021 at 7:19 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed,  7 Apr 2021 08:11:54 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > The warning() function is used in a lot of places in the libracefs
> > library, and it is implemented as a dummy weak function. There is a
> > weak implementation of the same function in traceevent library, which
> > could cause a problem when both are used in an application.
> > Replaced warning() with tracefs_warning() and implemented it as a
> > weak function, specific to this library.
> > Added a __weak define in the library internal header file.
>
> I've been thinking about this more, and I think we only want one function
> to override, and that would be:
>
> void __weak tep_vwarning(const char *name, const char *fmt, va_list ap)
> {
>         if (errno)
>                 perror(name);
>
>         fprintf(stderr, "  ");
>         vfprintf(stderr, fmt, ap);
>         fprintf(stderr, "\n");
> }
>
I understood that idea from your comments, but have a few concerns:

1. That way we create a dependency, not logical to the user of the libraries.
2. That print functionality is not something logically specific to the
libtraceevent, it is not related to the main purpose of this library.
3. A weak function specific to each library is a more straightforward
way and the user has the flexibility to control warnings per library.
The overhead to add library specific wrappers to those weak functions
is not so big.

[...]


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] libtracefs: Implement tracefs_warning()

[Steven Rostedt]

On Wed, 7 Apr 2021 19:46:43 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:


> I understood that idea from your comments, but have a few concerns:
> 
> 1. That way we create a dependency, not logical to the user of the libraries.

What dependency?

> 2. That print functionality is not something logically specific to the
> libtraceevent, it is not related to the main purpose of this library.

Not sure what you mean by that?

> 3. A weak function specific to each library is a more straightforward
> way and the user has the flexibility to control warnings per library.
> The overhead to add library specific wrappers to those weak functions
> is not so big.

It's an unnecessary burden. You may add libtracecmd, and want to capture
all the warnings and overwrite tracecmd_warning(), but then see that
there's other warnings coming from libtraceevent and libtracefs that you
never included (because libtracecmd pulled them in). And it's not obvious
how to deal with them.

-- Steve

Re: [PATCH] libtracefs: Implement tracefs_warning()

[Steven Rostedt]

On Wed, 7 Apr 2021 12:54:32 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > 3. A weak function specific to each library is a more straightforward
> > way and the user has the flexibility to control warnings per library.
> > The overhead to add library specific wrappers to those weak functions
> > is not so big.  
> 
> It's an unnecessary burden. You may add libtracecmd, and want to capture
> all the warnings and overwrite tracecmd_warning(), but then see that
> there's other warnings coming from libtraceevent and libtracefs that you
> never included (because libtracecmd pulled them in). And it's not obvious
> how to deal with them.

I'm fine if you want to make them all weak, but I really like to have a
single function that can control all of them.

-- Steve

Re: [PATCH] libtracefs: Implement tracefs_warning()

[Tzvetomir Stoyanov]

On Wed, Apr 7, 2021 at 7:55 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 7 Apr 2021 12:54:32 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > > 3. A weak function specific to each library is a more straightforward
> > > way and the user has the flexibility to control warnings per library.
> > > The overhead to add library specific wrappers to those weak functions
> > > is not so big.
> >
> > It's an unnecessary burden. You may add libtracecmd, and want to capture
> > all the warnings and overwrite tracecmd_warning(), but then see that
> > there's other warnings coming from libtraceevent and libtracefs that you
> > never included (because libtracecmd pulled them in). And it's not obvious
> > how to deal with them.
>
> I'm fine if you want to make them all weak, but I really like to have a
> single function that can control all of them.

I'll submit the next version with weak functions for each library and
all of them using the __weak tep_vwarning() for the printing.

>
> -- Steve



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] libtracefs: Implement tracefs_warning()

[Steven Rostedt]

On Wed, 7 Apr 2021 19:59:39 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> I'll submit the next version with weak functions for each library and
> all of them using the __weak tep_vwarning() for the printing.

Sounds good.

Thanks!

-- Steve