On Mon, Mar 22, 2021 at 5:33 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Mon, 22 Mar 2021 13:54:22 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
|