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

walter harms
2015-06-09T12:37:07Z

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;