Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

From: Arnaldo Carvalho de Melo
Date: Tue Mar 06 2018 - 15:25:54 EST


Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
> In preparation for supporting AUX area sampling buffers,
> auxtrace_queues__add_buffer() needs to be more generic. To that end, move
> memory allocation for struct buffer into it.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
> tools/perf/util/auxtrace.c | 54 +++++++++++++++++++++-------------------------
> 1 file changed, 24 insertions(+), 30 deletions(-)
>
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index fb357a00dd86..e1aff91c54a8 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct auxtrace_queues *queues,
> struct auxtrace_buffer *buffer,
> struct auxtrace_buffer **buffer_ptr)
> {
> - int err;
> + int err = -ENOMEM;
> +
> + buffer = memdup(buffer, sizeof(*buffer));

this is a bit strange, why not make buffer a local variable in this
function then?

> + if (!buffer)
> + return -ENOMEM;
>
> if (session->one_mmap) {
> buffer->data = buffer->data_offset - session->one_mmap_offset +
> @@ -316,24 +320,28 @@ static int auxtrace_queues__add_buffer(struct auxtrace_queues *queues,
> } else if (perf_data__is_pipe(session->data)) {
> buffer->data = auxtrace_copy_data(buffer->size, session);
> if (!buffer->data)
> - return -ENOMEM;
> + goto out_free;
> buffer->data_needs_freeing = true;
> } else if (BITS_PER_LONG == 32 &&
> buffer->size > BUFFER_LIMIT_FOR_32_BIT) {
> err = auxtrace_queues__split_buffer(queues, idx, buffer);
> if (err)
> - return err;
> + goto out_free;
> }
>
> err = auxtrace_queues__queue_buffer(queues, idx, buffer);
> if (err)
> - return err;
> + goto out_free;
>
> /* FIXME: Doesn't work for split buffer */
> if (buffer_ptr)
> *buffer_ptr = buffer;
>
> return 0;
> +
> +out_free:
> + auxtrace_buffer__free(buffer);
> + return err;
> }
>
> static bool filter_cpu(struct perf_session *session, int cpu)
> @@ -348,36 +356,22 @@ int auxtrace_queues__add_event(struct auxtrace_queues *queues,
> union perf_event *event, off_t data_offset,
> struct auxtrace_buffer **buffer_ptr)
> {
> - struct auxtrace_buffer *buffer;
> - unsigned int idx;
> - int err;
> + struct auxtrace_buffer buffer = {
> + .pid = -1,
> + .tid = event->auxtrace.tid,
> + .cpu = event->auxtrace.cpu,
> + .data_offset = data_offset,
> + .offset = event->auxtrace.offset,
> + .reference = event->auxtrace.reference,
> + .size = event->auxtrace.size,
> + };
> + unsigned int idx = event->auxtrace.idx;
>
> if (filter_cpu(session, event->auxtrace.cpu))
> return 0;
>
> - buffer = zalloc(sizeof(struct auxtrace_buffer));
> - if (!buffer)
> - return -ENOMEM;
> -
> - buffer->pid = -1;
> - buffer->tid = event->auxtrace.tid;
> - buffer->cpu = event->auxtrace.cpu;
> - buffer->data_offset = data_offset;
> - buffer->offset = event->auxtrace.offset;
> - buffer->reference = event->auxtrace.reference;
> - buffer->size = event->auxtrace.size;
> - idx = event->auxtrace.idx;
> -
> - err = auxtrace_queues__add_buffer(queues, session, idx, buffer,
> - buffer_ptr);
> - if (err)
> - goto out_err;
> -
> - return 0;
> -
> -out_err:
> - auxtrace_buffer__free(buffer);
> - return err;
> + return auxtrace_queues__add_buffer(queues, session, idx, &buffer,
> + buffer_ptr);
> }
>
> static int auxtrace_queues__add_indexed_event(struct auxtrace_queues *queues,
> --
> 1.9.1