Re: [PATCH v2] perf tools: Add error handling

[walter harms <wharms@bfs.de>]

Am 09.06.2015 09:49, schrieb Firo Yang:
> Add error handling code for snprintf and rename in check_backup.
> 
> Signed-off-by: Firo Yang <firogm@gmail.com>
> ---
> Since there is  no suitable error code snprintf, I just return the 
> value returned by snprintf.
> 
>  tools/perf/util/data.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index 1921942..26ab45a 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -32,15 +32,26 @@ static bool check_pipe(struct perf_data_file *file)
>  
>  static int check_backup(struct perf_data_file *file)
>  {
> +	int ret;
>  	struct stat st;
>  
>  	if (!stat(file->path, &st) && st.st_size) {
> -		/* TODO check errors properly */
>  		char oldname[PATH_MAX];
> -		snprintf(oldname, sizeof(oldname), "%s.old",
> +		ret = snprintf(oldname, sizeof(oldname), "%s.old",
>  			 file->path);


i am a big fan of asprintf() since that allows you to have just the right size.
Just do not forget to free the allocated buffer.


> +		if (ret < 0) {
> +			pr_err("failed to make name %s.old\n", file->path);
> +			return ret;
> +		}
> +
>  		unlink(oldname);
> -		rename(file->path, oldname);
> +
> +		ret = rename(file->path, oldname);
> +		if (ret < 0) {
> +			pr_err("failed to rename %s to %s\n", file->path,
> +				oldname);
> +			return -errno;
> +		}

short: it does not work this way. rename will return EXDEV if source
and destination are on two file systems. (See man 2 rename)
perfect recoverable.
(on the other side: can this happen ? i do not know)


just my 2 cents
re,
 wh

>  	}
>  
>  	return 0;