[PATCH 0/4] zbd: clean up libzbc ioengine and tests

[PATCH 0/4] zbd: clean up libzbc ioengine and tests

[Dmitry Fomichev]

This series makes a few minor, but IMO useful changes in libzbc
ioengine and ZBD test script.

Dmitry Fomichev (4):
  libzbc: cleanup init code
  libzbc: fix whitespace errors
  t/zbd: beautify test script output
  t/zbd: make the test script easier to terminate

 engines/libzbc.c       | 33 +++++++++++++++++----------------
 t/zbd/test-zbd-support | 20 ++++++++++++++++++--
 2 files changed, 35 insertions(+), 18 deletions(-)

-- 
2.21.0

[PATCH 1/4] libzbc: cleanup init code

[Dmitry Fomichev]

Make sure every allocated data structure gets freed in case of
unsuccessful libzbc ioengine initialization.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 engines/libzbc.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/engines/libzbc.c b/engines/libzbc.c
index 8c682de6..2f6b4583 100644
--- a/engines/libzbc.c
+++ b/engines/libzbc.c
@@ -92,15 +92,12 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 	if (ret) {
 		log_err("%s: zbc_open() failed, err=%d\n",
 			f->file_name, ret);
-		return ret;
+		goto err;
 	}
 
 	ret = libzbc_get_dev_info(ld, f);
-	if (ret) {
-		zbc_close(ld->zdev);
-		free(ld);
-		return ret;
-	}
+	if (ret)
+		goto err_close;
 
 	td->io_ops_data = ld;
 out:
@@ -108,6 +105,12 @@ out:
 		*p_ld = ld;
 
 	return 0;
+
+err_close:
+	zbc_close(ld->zdev);
+err:
+	free(ld);
+	return ret;
 }
 
 static int libzbc_close_dev(struct thread_data *td)
-- 
2.21.0

[PATCH 2/4] libzbc: fix whitespace errors

[Dmitry Fomichev]

Make checkpatch happy... no functional change.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 engines/libzbc.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/engines/libzbc.c b/engines/libzbc.c
index 2f6b4583..9e568334 100644
--- a/engines/libzbc.c
+++ b/engines/libzbc.c
@@ -47,7 +47,7 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 			   struct libzbc_data **p_ld)
 {
 	struct libzbc_data *ld = td->io_ops_data;
-        int ret, flags = OS_O_DIRECT;
+	int ret, flags = OS_O_DIRECT;
 
 	if (ld) {
 		/* Already open */
@@ -61,7 +61,7 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 		return -EINVAL;
 	}
 
-        if (td_write(td)) {
+	if (td_write(td)) {
 		if (!read_only)
 			flags |= O_RDWR;
 	} else if (td_read(td)) {
@@ -71,17 +71,15 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
 			flags |= O_RDONLY;
 	} else if (td_trim(td)) {
 		td_verror(td, EINVAL, "libzbc does not support trim");
-                log_err("%s: libzbc does not support trim\n",
-                        f->file_name);
-                return -EINVAL;
+		log_err("%s: libzbc does not support trim\n", f->file_name);
+		return -EINVAL;
 	}
 
-        if (td->o.oatomic) {
+	if (td->o.oatomic) {
 		td_verror(td, EINVAL, "libzbc does not support O_ATOMIC");
-                log_err("%s: libzbc does not support O_ATOMIC\n",
-                        f->file_name);
-                return -EINVAL;
-        }
+		log_err("%s: libzbc does not support O_ATOMIC\n", f->file_name);
+		return -EINVAL;
+	}
 
 	ld = calloc(1, sizeof(*ld));
 	if (!ld)
-- 
2.21.0

[PATCH 3/4] t/zbd: beautify test script output

[Dmitry Fomichev]

The test printout columns are better aligned now. Also, the test
result, PASS/FAIL, is now color-coded and that makes it easier
to spot failures.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 t/zbd/test-zbd-support | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index de05f438..51b05dfd 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -929,19 +929,31 @@ logfile=$0.log
 
 passed=0
 failed=0
+if [ -t 1 ]; then
+    red="\e[1;31m"
+    green="\e[1;32m"
+    end="\e[m"
+else
+    red=""
+    green=""
+    end=""
+fi
 rc=0
+
 for test_number in "${tests[@]}"; do
     rm -f "${logfile}.${test_number}"
-    echo -n "Running test $test_number ... "
+    echo -n "Running test $(printf "%02d" $test_number) ... "
     if eval "test$test_number"; then
 	status="PASS"
+	cc_status="${green}${status}${end}"
 	((passed++))
     else
 	status="FAIL"
+	cc_status="${red}${status}${end}"
 	((failed++))
 	rc=1
     fi
-    echo "$status"
+    echo -e "$cc_status"
     echo "$status" >> "${logfile}.${test_number}"
 done
 
-- 
2.21.0

[PATCH 4/4] t/zbd: make the test script easier to terminate

[Dmitry Fomichev]

Very often, it takes more than one ^C to terminate test-zbd-support
script. Just a single ^C does end the test that is currently being
executed, but then the script proceeds to the next test. This commit
adds a simple signal handler to exit the test loop after receiving
a Ctrl-C.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 t/zbd/test-zbd-support | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index 51b05dfd..4001be3b 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -940,6 +940,9 @@ else
 fi
 rc=0
 
+intr=0
+trap 'intr=1' SIGINT
+
 for test_number in "${tests[@]}"; do
     rm -f "${logfile}.${test_number}"
     echo -n "Running test $(printf "%02d" $test_number) ... "
@@ -955,6 +958,7 @@ for test_number in "${tests[@]}"; do
     fi
     echo -e "$cc_status"
     echo "$status" >> "${logfile}.${test_number}"
+    [ $intr -ne 0 ] && exit 1
 done
 
 echo "$passed tests passed"
-- 
2.21.0

Re: [PATCH 1/4] libzbc: cleanup init code

[Damien Le Moal]

On 2020/05/26 6:33, Dmitry Fomichev wrote:
> Make sure every allocated data structure gets freed in case of
> unsuccessful libzbc ioengine initialization.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  engines/libzbc.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/engines/libzbc.c b/engines/libzbc.c
> index 8c682de6..2f6b4583 100644
> --- a/engines/libzbc.c
> +++ b/engines/libzbc.c
> @@ -92,15 +92,12 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
>  	if (ret) {
>  		log_err("%s: zbc_open() failed, err=%d\n",
>  			f->file_name, ret);
> -		return ret;
> +		goto err;
>  	}
>  
>  	ret = libzbc_get_dev_info(ld, f);
> -	if (ret) {
> -		zbc_close(ld->zdev);
> -		free(ld);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_close;
>  
>  	td->io_ops_data = ld;
>  out:
> @@ -108,6 +105,12 @@ out:
>  		*p_ld = ld;
>  
>  	return 0;
> +
> +err_close:
> +	zbc_close(ld->zdev);
> +err:
> +	free(ld);
> +	return ret;
>  }
>  
>  static int libzbc_close_dev(struct thread_data *td)
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] libzbc: fix whitespace errors

[Damien Le Moal]

On 2020/05/26 6:33, Dmitry Fomichev wrote:
> Make checkpatch happy... no functional change.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  engines/libzbc.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/engines/libzbc.c b/engines/libzbc.c
> index 2f6b4583..9e568334 100644
> --- a/engines/libzbc.c
> +++ b/engines/libzbc.c
> @@ -47,7 +47,7 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
>  			   struct libzbc_data **p_ld)
>  {
>  	struct libzbc_data *ld = td->io_ops_data;
> -        int ret, flags = OS_O_DIRECT;
> +	int ret, flags = OS_O_DIRECT;
>  
>  	if (ld) {
>  		/* Already open */
> @@ -61,7 +61,7 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
>  		return -EINVAL;
>  	}
>  
> -        if (td_write(td)) {
> +	if (td_write(td)) {
>  		if (!read_only)
>  			flags |= O_RDWR;
>  	} else if (td_read(td)) {
> @@ -71,17 +71,15 @@ static int libzbc_open_dev(struct thread_data *td, struct fio_file *f,
>  			flags |= O_RDONLY;
>  	} else if (td_trim(td)) {
>  		td_verror(td, EINVAL, "libzbc does not support trim");
> -                log_err("%s: libzbc does not support trim\n",
> -                        f->file_name);
> -                return -EINVAL;
> +		log_err("%s: libzbc does not support trim\n", f->file_name);
> +		return -EINVAL;
>  	}
>  
> -        if (td->o.oatomic) {
> +	if (td->o.oatomic) {
>  		td_verror(td, EINVAL, "libzbc does not support O_ATOMIC");
> -                log_err("%s: libzbc does not support O_ATOMIC\n",
> -                        f->file_name);
> -                return -EINVAL;
> -        }
> +		log_err("%s: libzbc does not support O_ATOMIC\n", f->file_name);
> +		return -EINVAL;
> +	}
>  
>  	ld = calloc(1, sizeof(*ld));
>  	if (!ld)
> 

Jens,

Not sure if you take white-space patches. But if you do:

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] t/zbd: beautify test script output

[Damien Le Moal]

On 2020/05/26 6:33, Dmitry Fomichev wrote:
> The test printout columns are better aligned now. Also, the test
> result, PASS/FAIL, is now color-coded and that makes it easier
> to spot failures.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  t/zbd/test-zbd-support | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
> index de05f438..51b05dfd 100755
> --- a/t/zbd/test-zbd-support
> +++ b/t/zbd/test-zbd-support
> @@ -929,19 +929,31 @@ logfile=$0.log
>  
>  passed=0
>  failed=0
> +if [ -t 1 ]; then
> +    red="\e[1;31m"
> +    green="\e[1;32m"
> +    end="\e[m"
> +else
> +    red=""
> +    green=""
> +    end=""
> +fi
>  rc=0
> +
>  for test_number in "${tests[@]}"; do
>      rm -f "${logfile}.${test_number}"
> -    echo -n "Running test $test_number ... "
> +    echo -n "Running test $(printf "%02d" $test_number) ... "
>      if eval "test$test_number"; then
>  	status="PASS"
> +	cc_status="${green}${status}${end}"
>  	((passed++))
>      else
>  	status="FAIL"
> +	cc_status="${red}${status}${end}"
>  	((failed++))
>  	rc=1
>      fi
> -    echo "$status"
> +    echo -e "$cc_status"
>      echo "$status" >> "${logfile}.${test_number}"
>  done
>  
> 

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] t/zbd: make the test script easier to terminate

[Damien Le Moal]

On 2020/05/26 6:33, Dmitry Fomichev wrote:
> Very often, it takes more than one ^C to terminate test-zbd-support
> script. Just a single ^C does end the test that is currently being
> executed, but then the script proceeds to the next test. This commit
> adds a simple signal handler to exit the test loop after receiving
> a Ctrl-C.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  t/zbd/test-zbd-support | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
> index 51b05dfd..4001be3b 100755
> --- a/t/zbd/test-zbd-support
> +++ b/t/zbd/test-zbd-support
> @@ -940,6 +940,9 @@ else
>  fi
>  rc=0
>  
> +intr=0
> +trap 'intr=1' SIGINT
> +
>  for test_number in "${tests[@]}"; do
>      rm -f "${logfile}.${test_number}"
>      echo -n "Running test $(printf "%02d" $test_number) ... "
> @@ -955,6 +958,7 @@ for test_number in "${tests[@]}"; do
>      fi
>      echo -e "$cc_status"
>      echo "$status" >> "${logfile}.${test_number}"
> +    [ $intr -ne 0 ] && exit 1
>  done
>  
>  echo "$passed tests passed"
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/4] zbd: clean up libzbc ioengine and tests

[Jens Axboe]

On 5/25/20 3:32 PM, Dmitry Fomichev wrote:
> This series makes a few minor, but IMO useful changes in libzbc
> ioengine and ZBD test script.
> 
> Dmitry Fomichev (4):
>   libzbc: cleanup init code
>   libzbc: fix whitespace errors
>   t/zbd: beautify test script output
>   t/zbd: make the test script easier to terminate
> 
>  engines/libzbc.c       | 33 +++++++++++++++++----------------
>  t/zbd/test-zbd-support | 20 ++++++++++++++++++--
>  2 files changed, 35 insertions(+), 18 deletions(-)

Applied, thanks. BTW, since Damien asked, I'm fine with whitespace
changes as long as they are separate. Don't mix them with functional
changes. Ditto coding style changes.

-- 
Jens Axboe