Re: [PATCH 2/2] perf header: Less function calls in read_event_desc() after error detection

[SF Markus Elfring]

>> The functions "free" and "free_event_desc" were called in a few cases by the
>> function "read_event_desc" during error handling even if the passed variable
>> contained a null pointer.
> 
> And that is not a problem, free(NULL) is perfectly valid, as is valid to
> call free_event_desc(NULL).

I hope that inefficient exception handling can be fixed.


> But if you want to avoid those extra NULL checks done at those functions,
> then add a new out: label that just do 'return events;' that is NULL, etc.

I adjusted the jump labels in the affected function already.


>> This implementation detail could be improved by the adjustment of jump targets.
>> Drop unnecessary initialisations for the variables "buf" and "events" then.
>>
>> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  tools/perf/util/header.c | 29 +++++++++++++++--------------
>>  1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index 03ace57..8071163 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -978,9 +978,9 @@ static void free_event_desc(struct perf_evsel *events)
>>  static struct perf_evsel *
>>  read_event_desc(struct perf_header *ph, int fd)
>>  {
>> -	struct perf_evsel *evsel, *events = NULL;
>> +	struct perf_evsel *evsel, *events;
>>  	u64 *id;
>> -	void *buf = NULL;
>> +	void *buf;
>>  	u32 nre, sz, nr, i, j;
>>  	ssize_t ret;
>>  	size_t msz;
>> @@ -988,14 +988,14 @@ read_event_desc(struct perf_header *ph, int fd)
>>  	/* number of events */
>>  	ret = readn(fd, &nre, sizeof(nre));
>>  	if (ret != (ssize_t)sizeof(nre))
>> -		goto error;
>> +		return NULL;
> 
> Please leave the single point of exit, i.e. this should 'goto out:' and
> the out: label will return events, that is set to NULL

Does my update suggestion fit to the wording "If there is no cleanup needed
then just return directly." from the Linux coding style documentation?


>>  	if (ph->needs_swap)
>>  		nre = bswap_32(nre);
>>  
>>  	ret = readn(fd, &sz, sizeof(sz));
>>  	if (ret != (ssize_t)sizeof(sz))
>> -		goto error;
>> +		return NULL;
>>  
>>  	if (ph->needs_swap)
>>  		sz = bswap_32(sz);
>> @@ -1003,12 +1003,12 @@ read_event_desc(struct perf_header *ph, int fd)
>>  	/* buffer to hold on file attr struct */
>>  	buf = malloc(sz);
>>  	if (!buf)
>> -		goto error;
>> +		return NULL;
>>  
>>  	/* the last event terminates with evsel->attr.size == 0: */
>>  	events = calloc(nre + 1, sizeof(*events));
>>  	if (!events)
>> -		goto error;
>> +		goto free_buffer;
>>  
>>  	msz = sizeof(evsel->attr);
>>  	if (sz < msz)
>> @@ -1023,7 +1023,7 @@ read_event_desc(struct perf_header *ph, int fd)
>>  		 */
>>  		ret = readn(fd, buf, sz);
>>  		if (ret != (ssize_t)sz)
>> -			goto error;
>> +			goto free_events;
>>  
>>  		if (ph->needs_swap)
>>  			perf_event__attr_swap(buf);
>> @@ -1032,7 +1032,7 @@ read_event_desc(struct perf_header *ph, int fd)
>>  
>>  		ret = readn(fd, &nr, sizeof(nr));
>>  		if (ret != (ssize_t)sizeof(nr))
>> -			goto error;
>> +			goto free_events;
>>  
>>  		if (ph->needs_swap) {
>>  			nr = bswap_32(nr);
>> @@ -1046,26 +1046,27 @@ read_event_desc(struct perf_header *ph, int fd)
>>  
>>  		id = calloc(nr, sizeof(*id));
>>  		if (!id)
>> -			goto error;
>> +			goto free_events;
>>  		evsel->ids = nr;
>>  		evsel->id = id;
>>  
>>  		for (j = 0 ; j < nr; j++) {
>>  			ret = readn(fd, id, sizeof(*id));
>>  			if (ret != (ssize_t)sizeof(*id))
>> -				goto error;
>> +				goto free_events;
>>  			if (ph->needs_swap)
>>  				*id = bswap_64(*id);
>>  			id++;
>>  		}
>>  	}
>> -out:
>> +
>>  	free(buf);
>>  	return events;
>> -error:
>> +free_events:
>>  	free_event_desc(events);
>> -	events = NULL;
>> -	goto out;
>> +free_buffer:
>> +	free(buf);
>> +	return NULL;
>>  }
>>  
>>  static int __desc_attr__fprintf(FILE *fp, const char *name, const char *val,
>> -- 
>> 2.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/