[PATCH] trace-cmd: Remove all die()s from trace-cmd library

[PATCH] trace-cmd: Remove all die()s from trace-cmd library

[Tzvetomir Stoyanov (VMware)]

The die() call is used for a fatal error. It terminates the current
program with exit(). The program should not be terminated from a library
routine, the die() call should not be used in trace-cmd library context.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 10 ++++------
 lib/trace-cmd/trace-util.c  | 21 +--------------------
 2 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 2093a3dc..5398154f 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -1088,7 +1088,7 @@ static void __free_page(struct tracecmd_input *handle, struct page *page)
 	int index;
 
 	if (!page->ref_count)
-		die("Page ref count is zero!\n");
+		return;
 
 	page->ref_count--;
 	if (page->ref_count)
@@ -1147,7 +1147,7 @@ void tracecmd_free_record(struct tep_record *record)
 		return;
 
 	if (!record->ref_count)
-		die("record ref count is zero!");
+		return;
 
 	record->ref_count--;
 
@@ -1155,7 +1155,7 @@ void tracecmd_free_record(struct tep_record *record)
 		return;
 
 	if (record->locked)
-		die("freeing record when it is locked!");
+		return;
 
 	record->data = NULL;
 
@@ -1318,7 +1318,6 @@ static int get_page(struct tracecmd_input *handle, int cpu,
 
 	if (offset & (handle->page_size - 1)) {
 		errno = -EINVAL;
-		die("bad page offset %llx", offset);
 		return -1;
 	}
 
@@ -1326,7 +1325,6 @@ static int get_page(struct tracecmd_input *handle, int cpu,
 	    offset > handle->cpu_data[cpu].file_offset +
 	    handle->cpu_data[cpu].file_size) {
 		errno = -EINVAL;
-		die("bad page offset %llx", offset);
 		return -1;
 	}
 
@@ -1892,7 +1890,7 @@ tracecmd_peek_data(struct tracecmd_input *handle, int cpu)
 
 		record = handle->cpu_data[cpu].next;
 		if (!record->data)
-			die("Something freed the record");
+			return NULL;
 
 		if (handle->cpu_data[cpu].timestamp == record->ts)
 			return record;
diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
index 538adbc2..ce2e7afb 100644
--- a/lib/trace-cmd/trace-util.c
+++ b/lib/trace-cmd/trace-util.c
@@ -378,25 +378,6 @@ void __noreturn __die(const char *fmt, ...)
 	va_end(ap);
 }
 
-void __weak __noreturn die(const char *fmt, ...)
-{
-	va_list ap;
-
-	va_start(ap, fmt);
-	__vdie(fmt, ap);
-	va_end(ap);
-}
-
-void __weak *malloc_or_die(unsigned int size)
-{
-	void *data;
-
-	data = malloc(size);
-	if (!data)
-		die("malloc");
-	return data;
-}
-
 #define LOG_BUF_SIZE 1024
 static void __plog(const char *prefix, const char *fmt, va_list ap, FILE *fp)
 {
@@ -547,7 +528,7 @@ int tracecmd_count_cpus(void)
 
 	fp = fopen("/proc/cpuinfo", "r");
 	if (!fp)
-		die("Can not read cpuinfo");
+		return 0;
 
 	while ((r = getline(&pbuf, pn, fp)) >= 0) {
 		char *p;
-- 
2.30.2

Re: [PATCH] trace-cmd: Remove all die()s from trace-cmd library

[Steven Rostedt]

On Mon, 22 Mar 2021 13:54:22 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> The die() call is used for a fatal error. It terminates the current
> program with exit(). The program should not be terminated from a library
> routine, the die() call should not be used in trace-cmd library context.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 10 ++++------
>  lib/trace-cmd/trace-util.c  | 21 +--------------------
>  2 files changed, 5 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 2093a3dc..5398154f 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -1088,7 +1088,7 @@ static void __free_page(struct tracecmd_input *handle, struct page *page)
>  	int index;
>  
>  	if (!page->ref_count)
> -		die("Page ref count is zero!\n");
> +		return;
>  
>  	page->ref_count--;
>  	if (page->ref_count)
> @@ -1147,7 +1147,7 @@ void tracecmd_free_record(struct tep_record *record)
>  		return;
>  
>  	if (!record->ref_count)
> -		die("record ref count is zero!");
> +		return;
>  
>  	record->ref_count--;
>  
> @@ -1155,7 +1155,7 @@ void tracecmd_free_record(struct tep_record *record)
>  		return;
>  
>  	if (record->locked)
> -		die("freeing record when it is locked!");
> +		return;


The above is really valuable in debugging. We should change them from die()
to a new function, perhaps called "bug()" that works just like die, but
will only die if the user explicitly asks to do so.

Because these "die()" calls have found several bugs in the past.

These are probably the few places I would say do a fprintf(stderr, ...) as
well even when not debugging, because when they are hit, it means something
horrible went wrong.

>  
>  	record->data = NULL;
>  
> @@ -1318,7 +1318,6 @@ static int get_page(struct tracecmd_input *handle, int cpu,
>  
>  	if (offset & (handle->page_size - 1)) {
>  		errno = -EINVAL;
> -		die("bad page offset %llx", offset);
>  		return -1;
>  	}
>  
> @@ -1326,7 +1325,6 @@ static int get_page(struct tracecmd_input *handle, int cpu,
>  	    offset > handle->cpu_data[cpu].file_offset +
>  	    handle->cpu_data[cpu].file_size) {
>  		errno = -EINVAL;
> -		die("bad page offset %llx", offset);
>  		return -1;
>  	}
>  
> @@ -1892,7 +1890,7 @@ tracecmd_peek_data(struct tracecmd_input *handle, int cpu)
>  
>  		record = handle->cpu_data[cpu].next;
>  		if (!record->data)
> -			die("Something freed the record");
> +			return NULL;

I know I hate it when libraries do output, but the above really should be
at least printed (maybe not crash). Because they are equivalent to a
"WARN_ON()" in the kernel.

And when I screw something up, these are the first notifications I see
telling me I screwed something up ;-)

I know I told you that we should get rid of all prints from the library, as
I hate seeing them in applications (libgtk is horrible with that). But now
seeing what is calling it, I change my mind about it.


>  
>  		if (handle->cpu_data[cpu].timestamp == record->ts)
>  			return record;
> diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
> index 538adbc2..ce2e7afb 100644
> --- a/lib/trace-cmd/trace-util.c
> +++ b/lib/trace-cmd/trace-util.c
> @@ -378,25 +378,6 @@ void __noreturn __die(const char *fmt, ...)
>  	va_end(ap);
>  }
>  
> -void __weak __noreturn die(const char *fmt, ...)
> -{
> -	va_list ap;
> -
> -	va_start(ap, fmt);
> -	__vdie(fmt, ap);
> -	va_end(ap);
> -}
> -
> -void __weak *malloc_or_die(unsigned int size)
> -{
> -	void *data;
> -
> -	data = malloc(size);
> -	if (!data)
> -		die("malloc");
> -	return data;
> -}

We should make these internal functions, that always call warning() (which
we probably should change to tracecmd_warning()), and with some flag set,
would still crash the application.

-- Steve


> -
>  #define LOG_BUF_SIZE 1024
>  static void __plog(const char *prefix, const char *fmt, va_list ap, FILE *fp)
>  {
> @@ -547,7 +528,7 @@ int tracecmd_count_cpus(void)
>  
>  	fp = fopen("/proc/cpuinfo", "r");
>  	if (!fp)
> -		die("Can not read cpuinfo");
> +		return 0;
>  
>  	while ((r = getline(&pbuf, pn, fp)) >= 0) {
>  		char *p;

Re: [PATCH] trace-cmd: Remove all die()s from trace-cmd library

[Tzvetomir Stoyanov]

On Mon, Mar 22, 2021 at 5:33 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 22 Mar 2021 13:54:22 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > The die() call is used for a fatal error. It terminates the current
> > program with exit(). The program should not be terminated from a library
> > routine, the die() call should not be used in trace-cmd library context.
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  lib/trace-cmd/trace-input.c | 10 ++++------
> >  lib/trace-cmd/trace-util.c  | 21 +--------------------
> >  2 files changed, 5 insertions(+), 26 deletions(-)
> >
> > diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> > index 2093a3dc..5398154f 100644
> > --- a/lib/trace-cmd/trace-input.c
> > +++ b/lib/trace-cmd/trace-input.c
> > @@ -1088,7 +1088,7 @@ static void __free_page(struct tracecmd_input *handle, struct page *page)
> >       int index;
> >
> >       if (!page->ref_count)
> > -             die("Page ref count is zero!\n");
> > +             return;
> >
> >       page->ref_count--;
> >       if (page->ref_count)
> > @@ -1147,7 +1147,7 @@ void tracecmd_free_record(struct tep_record *record)
> >               return;
> >
> >       if (!record->ref_count)
> > -             die("record ref count is zero!");
> > +             return;
> >
> >       record->ref_count--;
> >
> > @@ -1155,7 +1155,7 @@ void tracecmd_free_record(struct tep_record *record)
> >               return;
> >
> >       if (record->locked)
> > -             die("freeing record when it is locked!");
> > +             return;
>
>
> The above is really valuable in debugging. We should change them from die()
> to a new function, perhaps called "bug()" that works just like die, but
> will only die if the user explicitly asks to do so.
>
> Because these "die()" calls have found several bugs in the past.
>
> These are probably the few places I would say do a fprintf(stderr, ...) as
> well even when not debugging, because when they are hit, it means something
> horrible went wrong.
>
> >
> >       record->data = NULL;
> >
> > @@ -1318,7 +1318,6 @@ static int get_page(struct tracecmd_input *handle, int cpu,
> >
> >       if (offset & (handle->page_size - 1)) {
> >               errno = -EINVAL;
> > -             die("bad page offset %llx", offset);
> >               return -1;
> >       }
> >
> > @@ -1326,7 +1325,6 @@ static int get_page(struct tracecmd_input *handle, int cpu,
> >           offset > handle->cpu_data[cpu].file_offset +
> >           handle->cpu_data[cpu].file_size) {
> >               errno = -EINVAL;
> > -             die("bad page offset %llx", offset);
> >               return -1;
> >       }
> >
> > @@ -1892,7 +1890,7 @@ tracecmd_peek_data(struct tracecmd_input *handle, int cpu)
> >
> >               record = handle->cpu_data[cpu].next;
> >               if (!record->data)
> > -                     die("Something freed the record");
> > +                     return NULL;
>
> I know I hate it when libraries do output, but the above really should be
> at least printed (maybe not crash). Because they are equivalent to a
> "WARN_ON()" in the kernel.
>
> And when I screw something up, these are the first notifications I see
> telling me I screwed something up ;-)
>
> I know I told you that we should get rid of all prints from the library, as
> I hate seeing them in applications (libgtk is horrible with that). But now
> seeing what is calling it, I change my mind about it.
>
>
> >
> >               if (handle->cpu_data[cpu].timestamp == record->ts)
> >                       return record;
> > diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
> > index 538adbc2..ce2e7afb 100644
> > --- a/lib/trace-cmd/trace-util.c
> > +++ b/lib/trace-cmd/trace-util.c
> > @@ -378,25 +378,6 @@ void __noreturn __die(const char *fmt, ...)
> >       va_end(ap);
> >  }
> >
> > -void __weak __noreturn die(const char *fmt, ...)
> > -{
> > -     va_list ap;
> > -
> > -     va_start(ap, fmt);
> > -     __vdie(fmt, ap);
> > -     va_end(ap);
> > -}
> > -
> > -void __weak *malloc_or_die(unsigned int size)
> > -{
> > -     void *data;
> > -
> > -     data = malloc(size);
> > -     if (!data)
> > -             die("malloc");
> > -     return data;
> > -}
>
> We should make these internal functions, that always call warning() (which
> we probably should change to tracecmd_warning()), and with some flag set,
> would still crash the application.

I think those die()s should do warning() by default and real die() if
some compile time flag is set. I can send v2 with that change.

>
> -- Steve
>
>
> > -
> >  #define LOG_BUF_SIZE 1024
> >  static void __plog(const char *prefix, const char *fmt, va_list ap, FILE *fp)
> >  {
> > @@ -547,7 +528,7 @@ int tracecmd_count_cpus(void)
> >
> >       fp = fopen("/proc/cpuinfo", "r");
> >       if (!fp)
> > -             die("Can not read cpuinfo");
> > +             return 0;
> >
> >       while ((r = getline(&pbuf, pn, fp)) >= 0) {
> >               char *p;
>


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