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

[Arnaldo Carvalho de Melo]

Em Thu, Jun 25, 2015 at 06:24:09PM +0200, SF Markus Elfring escreveu:
> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> Date: Thu, 25 Jun 2015 17:50:37 +0200
> 
> 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).

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.

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

>  
>  	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/