Messages in this thread | | | Date | Thu, 26 Dec 2013 09:51:25 -0500 | From | David Ahern <> | Subject | Re: [PATCH 4/8] perf tools: Introduce struct perf_log |
| |
On 12/26/13, 12:38 AM, Namhyung Kim wrote: > diff --git a/tools/perf/util/log.c b/tools/perf/util/log.c > new file mode 100644 > index 000000000000..3838d49f82de > --- /dev/null > +++ b/tools/perf/util/log.c > @@ -0,0 +1,105 @@ > +#include <stdio.h> > +#include <stdlib.h> > +#include "util/debug.h" > + > +#define LINEMAP_GROW 128 > + > +struct perf_log perf_log = { > + .seen_newline = true, > +}; > + > +int perf_log_init(void)
Why return int if the rc is not checked? Failure here is not going to stop the perf command right?
> +{ > + FILE *fp; > + char name[] = "/tmp/perf-log-XXXXXX"; > + int fd = mkstemp(name); > + > + if (fd < 0) > + return -1; > + > + fp = fdopen(fd, "r+"); > + if (fp == NULL) { > + close(fd); > + return -1; > + } > + > + perf_log.fp = fp;
Add 'unlink(name);' here to ensure the file is removed regardless of how perf terminates.
> + > + return 0; > +} > + > +int perf_log_exit(void) > +{ > + FILE *fp = perf_log.fp; > + if (fp) > + fclose(fp); > + > + free(perf_log.linemap); > + > + perf_log.fp = NULL; > + perf_log.linemap = NULL; > + return 0; > +} > + > +static int grow_linemap(struct perf_log *log) > +{ > + off_t *newmap; > + int newsize = log->nr_alloc + LINEMAP_GROW; > + > + newmap = realloc(log->linemap, newsize * sizeof(*log->linemap)); > + if (newmap == NULL) > + return -1; > + > + log->nr_alloc = newsize; > + log->linemap = newmap; > + return 0; > +}
What's the point of linemap?
> + > +static int __add_to_linemap(struct perf_log *log, off_t idx) > +{ > + if (log->lines == log->nr_alloc) > + if (grow_linemap(log) < 0) > + return -1; > + > + log->linemap[log->lines++] = idx; > + return 0; > +} > + > +static void add_to_linemap(struct perf_log *log, const char *msg, off_t base) > +{ > + const char *pos; > + > + if (strlen(msg) == 0) > + return; > + > + if (log->seen_newline) { > + if (__add_to_linemap(log, base) < 0) > + return; > + } > + > + if ((pos = strchr(msg, '\n')) != NULL) { > + log->seen_newline = true; > + pos++; > + add_to_linemap(log, pos, base + (pos - msg)); > + } else { > + log->seen_newline = false; > + } > +} > + > +void perf_log_add(const char *msg) > +{ > + FILE *fp = perf_log.fp;
Don't assume every user of libperf calls perf_log_init() or that the file was actually created. i.e., add 'if (fp == NULL) return;'
> + off_t offset = ftello(fp); > + > + add_to_linemap(&perf_log, msg, offset); > + > + fwrite(msg, 1, strlen(msg), fp);
And if write fails?
> +} > + > +void perf_log_addv(const char *fmt, va_list ap) > +{ > + char buf[4096];
Add as an optimization add the fp != NULL check here too. Don't need to do the vsnprintf only to drop it.
> + > + vsnprintf(buf, sizeof(buf), fmt, ap); > + perf_log_add(buf); > +} >
What limits the size of the file - other than the obvious out of space in /tmp? Allow the file to grow without bounds in case a user wants the messages seems dangerous.
What about using a circular buffer instead?
David
| |