Re: [PATCH 4/8] perf tools: Introduce struct perf_log

[Namhyung Kim <namhyung@kernel.org>]

Hi David,

On Thu, 26 Dec 2013 09:51:25 -0500, David Ahern wrote:
> 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?

Right.  I'll add a debug print if it's failed.

>
>> +{
>> +	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.

Ah, okay.  I thought the mkstemp() unlinked the file after open()
returns.  It seems this is what tmpfile(3) does.  I'll switch to
tmpfile() then.

>
>> +
>> +	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?

To save an offset of each line.  We need to keep it in order to move to
an arbitraty line in the browser.

>
>> +
>> +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;'

Okay.

>
>
>> +	off_t offset = ftello(fp);
>> +
>> +	add_to_linemap(&perf_log, msg, offset);
>> +
>> +	fwrite(msg, 1, strlen(msg), fp);
>
> And if write fails?

Hmm.. it's a problem.  We might go back to original linemap position for
a failure case.  I'll save the offset and line number and restore them.

>
>> +}
>> +
>> +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.

Okay.

>
>> +
>> +	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.

Hmm. I don't have an idea what the reasonable size.  It's a temp file
anyway and usually it contains not much data - well, 'perf top -vvv' is
different and might need some surgery.

>
> What about using a circular buffer instead?

Instead of a file?  Yes, it's possible but I think it might not be a
good choice for the browser since it could alter the index during the
circulation so that the browser can confuse to update the entries.

Thanks,
Namhyung