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

[Arnaldo Carvalho de Melo]

Em Wed, Mar 07, 2018 at 10:06:50AM +0200, Adrian Hunter escreveu:
> On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
> > 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?
> 
> Do you mean the following?
> 
> 	struct auxtrace_buffer *new_buf;
> 
> 	new_buf = memdup(buffer, sizeof(*buffer));

I hadn't noticed that you were using buffer as both r and l value :-\

If all you want is to receive that buffer, duplicate it and then use
just the duplicate, not needing any reference to the original buffer,
then your code is correct, it just looked strange from a quick look, so
nevermind, I'll continue processing this one and the others.

Thanks,

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