Re: [PATCH v2 3/6] stat: Handle `ENOMEM` case in `__show_run_stats()`

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Wed, Apr 27, 2022 at 04:11:22PM +0700, Ammar Faizi wrote:
> This handles memory allocation failure and several improvements.
> 
> 1) Change `malloc(n * size)` to `calloc(n, size)`. This is to avoid
>    multiplication on `malloc()` because it doesn't do overflow check.
>    Also, `calloc()` zeroes the allocated memory, so we can omit the
>    zeroing elements of the array inside the loop.
> 
> 2) Make sure we are calling `free()` properly if we fail. Use goto to
>    do this.
> 
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>  stat.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/stat.c b/stat.c
> index 949af5ed..cc9be02e 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -2429,7 +2429,11 @@ void __show_run_stats(void)
>  	struct buf_output output[FIO_OUTPUT_NR];
>  	struct flist_head **opt_lists;
>  
> -	runstats = malloc(sizeof(struct group_run_stats) * (groupid + 1));
> +	runstats = calloc(groupid + 1, sizeof(*runstats));

Hello Ammar,

here you allocate runstats with calloc.

> +	if (!runstats) {
> +		log_err("fio: failed to allocate runstats\n");
> +		return;
> +	}
>  
>  	for (i = 0; i < groupid + 1; i++)
>  		init_group_run_stat(&runstats[i]);

Here you call init_group_run_stat() on each runstats,
which calls memset(). Seems a bit excessive to clear
the memory to zero twice.

If you intend to modify init_group_run_stat(), be careful,
as it is also called by client.c


Kind regards,
Niklas


> @@ -2454,14 +2458,21 @@ void __show_run_stats(void)
>  		nr_ts++;
>  	}
>  
> -	threadstats = malloc(nr_ts * sizeof(struct thread_stat));
> -	opt_lists = malloc(nr_ts * sizeof(struct flist_head *));
> +	threadstats = calloc(nr_ts, sizeof(*threadstats));
> +	if (!threadstats) {
> +		log_err("fio: failed to allocate threadstats\n");
> +		goto out_free_runstats;
> +	}
>  
> -	for (i = 0; i < nr_ts; i++) {
> -		init_thread_stat(&threadstats[i]);
> -		opt_lists[i] = NULL;
> +	opt_lists = calloc(nr_ts, sizeof(*opt_lists));
> +	if (!opt_lists) {
> +		log_err("fio: failed to allocate opt_lists\n");
> +		goto out_free_threadstats;
>  	}
>  
> +	for (i = 0; i < nr_ts; i++)
> +		init_thread_stat(&threadstats[i]);
> +
>  	init_per_prio_stats(threadstats, nr_ts);
>  
>  	j = 0;
> @@ -2709,15 +2720,18 @@ void __show_run_stats(void)
>  	fio_idle_prof_cleanup();
>  
>  	log_info_flush();
> -	free(runstats);
>  
>  	/* free arrays allocated by sum_thread_stats(), if any */
>  	for (i = 0; i < nr_ts; i++) {
>  		ts = &threadstats[i];
>  		free_clat_prio_stats(ts);
>  	}
> -	free(threadstats);
> +
>  	free(opt_lists);
> +out_free_threadstats:
> +	free(threadstats);
> +out_free_runstats:
> +	free(runstats);
>  }
>  
>  int __show_running_run_stats(void)
> -- 
> Ammar Faizi
>