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

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

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