[PATCH v2 0/6] Small fio cleanups and fixes

[PATCH v2 0/6] Small fio cleanups and fixes

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Hi Jens,

This is patchset v2. There 6 patches in this series. It contains small
cleanups and fixes:

- Patch 1 is just a trivial indentation fix.
- Patch 2, 3 are to add `ENOMEM` case error handling.
- Patch 4 is to replace `malloc()` + `memset()` with `calloc()`.
  Also, add ENOMEM hanling.
- Patch 5 is just a small optimization for json.
- Patch 6 is to fix warning from clang-15 when compiling the
  autogenerated file lex.yy.c.

## Changelog
v2:
  - Call `free()` properly when failure (use goto to do this) in
    patch #3.

  - Fix `calloc()` placement in patch #4.

Link v1: https://lore.kernel.org/fio/20220426212044.78898-1-ammarfaizi2@gnuweeb.org/T/


Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (6):
  backend: Fix indentation
  cgroup: Handle `ENOMEM` case on `malloc()` call
  stat: Handle `ENOMEM` case in `__show_run_stats()`
  engines/net: Replace `malloc()` + `memset()` with `calloc()`
  json: Change `if (!strlen(str))` to `if (!str[0])`
  Makefile: Suppress `-Wimplicit-fallthrough` when compiling `lex.yy`

 Makefile      |  6 +++++-
 backend.c     |  2 +-
 cgroup.c      |  4 ++++
 engines/net.c |  9 +++++----
 json.c        |  2 +-
 stat.c        | 30 ++++++++++++++++++++++--------
 6 files changed, 38 insertions(+), 15 deletions(-)


base-commit: 5f2d43188c2d65674aaba6280e2a87107e5d7099
-- 
Ammar Faizi

[PATCH v2 1/6] backend: Fix indentation

[Ammar Faizi]

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/backend.c b/backend.c
index 317e4f6c..071fd1d1 100644
--- a/backend.c
+++ b/backend.c
@@ -2021,7 +2021,7 @@ static void reap_threads(unsigned int *nr_running, uint64_t *t_rate,
 	for_each_td(td, i) {
 		int flags = 0;
 
-		 if (!strcmp(td->o.ioengine, "cpuio"))
+		if (!strcmp(td->o.ioengine, "cpuio"))
 			cputhreads++;
 		else
 			realthreads++;
-- 
Ammar Faizi

[PATCH v2 2/6] cgroup: Handle `ENOMEM` case on `malloc()` call

[Ammar Faizi]

Add error handling on `malloc()` call to prevent a NULL pointer
dereference.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 cgroup.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cgroup.c b/cgroup.c
index 77e31a4d..b559b70f 100644
--- a/cgroup.c
+++ b/cgroup.c
@@ -114,6 +114,8 @@ void cgroup_kill(struct flist_head *clist)
 static char *get_cgroup_root(struct thread_data *td, struct cgroup_mnt *mnt)
 {
 	char *str = malloc(64);
+	if (!str)
+		return NULL;
 
 	if (td->o.cgroup)
 		sprintf(str, "%s/%s", mnt->path, td->o.cgroup);
@@ -178,6 +180,8 @@ int cgroup_setup(struct thread_data *td, struct flist_head *clist, struct cgroup
 	 * Create container, if it doesn't exist
 	 */
 	root = get_cgroup_root(td, *mnt);
+	if (!root)
+		return 1;
 	if (mkdir(root, 0755) < 0) {
 		int __e = errno;
 
-- 
Ammar Faizi

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

[Ammar Faizi]

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

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

[Niklas Cassel]

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
> 

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

[Ammar Faizi]

On 4/27/22 4:35 PM, Niklas Cassel wrote:
> 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.

Agreed, I will CC you in the next versions.

Also, it seems like the vger kernel is messed up. My cover letter
and patch #5 don't appear on the lore.

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

Let's use `malloc()` for runstats instead of modifying
init_group_run_stat(), which is way much more simpler.

Will spin v3. Thank you!

-- 
Ammar Faizi

[PATCH v2 4/6] engines/net: Replace `malloc()` + `memset()` with `calloc()`

[Ammar Faizi]

Replace `malloc()` + `memset()` with `calloc()` to simplify the call.
`calloc()` zeroes the allocated memory, so we can avoid `memset()`.
Also, handle the `ENOMEM` case.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 engines/net.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/engines/net.c b/engines/net.c
index c6cec584..24c1463d 100644
--- a/engines/net.c
+++ b/engines/net.c
@@ -1370,9 +1370,9 @@ static int fio_netio_setup(struct thread_data *td)
 	}
 
 	if (!td->io_ops_data) {
-		nd = malloc(sizeof(*nd));
-
-		memset(nd, 0, sizeof(*nd));
+		nd = calloc(1, sizeof(*nd));
+		if (!nd)
+			return 1;
 		nd->listenfd = -1;
 		nd->pipes[0] = nd->pipes[1] = -1;
 		td->io_ops_data = nd;
@@ -1391,7 +1391,8 @@ static int fio_netio_setup_splice(struct thread_data *td)
 {
 	struct netio_data *nd;
 
-	fio_netio_setup(td);
+	if (fio_netio_setup(td))
+		return 1;
 
 	nd = td->io_ops_data;
 	if (nd) {
-- 
Ammar Faizi

[PATCH v2 5/6] json: Change `if (!strlen(str))` to `if (!str[0])`

[Ammar Faizi]

No need to traverse the whole string. Using `!strlen(str)` as a
*conditional expression* is effectively the same with `!str[0]`.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/json.c b/json.c
index cd3d5d74..8b650721 100644
--- a/json.c
+++ b/json.c
@@ -56,7 +56,7 @@ static char *strdup_escape(const char *str)
 	char *p, *ret;
 	int escapes;
 
-	if (!strlen(str))
+	if (!str[0])
 		return NULL;
 
 	escapes = 0;
-- 
Ammar Faizi

[PATCH v2 6/6] Makefile: Suppress `-Wimplicit-fallthrough` when compiling `lex.yy`

[Ammar Faizi]

lex.yy.c is an auto generated C file. When compiling with clang-15, we
got the following warning:

```
      CC lex.yy.o
  lex.yy.c:1444:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
                                  case EOB_ACT_END_OF_FILE:
                                  ^
  lex.yy.c:1444:5: note: insert '__attribute__((fallthrough));' to silence this warning
                                  case EOB_ACT_END_OF_FILE:
                                  ^
                                  __attribute__((fallthrough));
  lex.yy.c:1444:5: note: insert 'break;' to avoid fall-through
                                  case EOB_ACT_END_OF_FILE:
                                  ^
                                  break;
  1 warning generated.
```

There is nothing we can do to fix lex.yy.c since it's an auto generated
file. Fix this by appending `-Wno-implicit-fallthrough` when compiling
this file if we have `-Wimplicit-fallthrough` flag enabled.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e670c1f2..1e15a69e 100644
--- a/Makefile
+++ b/Makefile
@@ -530,8 +530,12 @@ else
 	$(QUIET_LEX)$(LEX) $<
 endif
 
+ifneq (,$(findstring -Wimplicit-fallthrough,$(CFLAGS)))
+LEX_YY_CFLAGS := -Wno-implicit-fallthrough
+endif
+
 lex.yy.o: lex.yy.c y.tab.h
-	$(QUIET_CC)$(CC) -o $@ $(CFLAGS) $(CPPFLAGS) -c $<
+	$(QUIET_CC)$(CC) -o $@ $(CFLAGS) $(CPPFLAGS) $(LEX_YY_CFLAGS) -c $<
 
 y.tab.o: y.tab.c y.tab.h
 	$(QUIET_CC)$(CC) -o $@ $(CFLAGS) $(CPPFLAGS) -c $<
-- 
Ammar Faizi