diff mbox series

[1/8] nvme/021: use consistent coding style

Message ID 1550618905-4112-2-git-send-email-chaitanya.kulkarni@wdc.com (mailing list archive)
State New, archived
Headers show
Series nvme: misc cleanups | expand

Commit Message

Chaitanya Kulkarni Feb. 19, 2019, 11:28 p.m. UTC 
---
 tests/nvme/021     | 15 ++++++---------
 tests/nvme/021.out |  1 -
 2 files changed, 6 insertions(+), 10 deletions(-)

Comments

Omar Sandoval Feb. 20, 2019, 6:10 p.m. UTC | #1 
On Tue, Feb 19, 2019 at 03:28:18PM -0800, Chaitanya Kulkarni wrote:
> ---
>  tests/nvme/021     | 15 ++++++---------
>  tests/nvme/021.out |  1 -
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/nvme/021 b/tests/nvme/021
> index 8e9512a..f204a2f 100755
> --- a/tests/nvme/021
> +++ b/tests/nvme/021
> @@ -38,11 +38,13 @@ test() {
>  	cat "/sys/block/${nvmedev}n1/uuid"
>  	cat "/sys/block/${nvmedev}n1/wwid"
>  
> -	nvme list > /dev/null 2>&1
> -
> -	rc=$?
> +	if ! nvme list | grep -q "${nvmedev}n1" > /dev/null 2>&1; then
> +		echo "Test Failed"
> +	else
> +		echo "Test complete"
> +	fi
>  
> -	nvme disconnect -n "${subsys_name}"
> +	nvme disconnect -n "${subsys_name}" > /dev/null 2>&1
>  
>  	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
>  	_remove_nvmet_subsystem "${subsys_name}"
> @@ -53,9 +55,4 @@ test() {
>  	modprobe -r nvme-loop
>  	modprobe -r nvmet
>  
> -	if [ $rc -ne 0 ]; then
> -		echo "Test Failed"
> -	else
> -		echo "Test complete"
> -	fi
>  }
> diff --git a/tests/nvme/021.out b/tests/nvme/021.out
> index 5fb0601..b6b1a7c 100644
> --- a/tests/nvme/021.out
> +++ b/tests/nvme/021.out
> @@ -1,5 +1,4 @@
>  Running nvme/021
>  91fdba0d-f87b-4c25-b80f-db7be1418b9e
>  uuid.91fdba0d-f87b-4c25-b80f-db7be1418b9e
> -NQN:blktests-subsystem-1 disconnected 1 controller(s)
>  Test complete
> -- 
> 1.8.3.1
> 

For all of these, I think it'd be useful to have a more descriptive
error, like so:

diff --git a/tests/nvme/021 b/tests/nvme/021
index 8e9512a..0b215c4 100755
--- a/tests/nvme/021
+++ b/tests/nvme/021
@@ -38,9 +38,9 @@ test() {
 	cat "/sys/block/${nvmedev}n1/uuid"
 	cat "/sys/block/${nvmedev}n1/wwid"
 
-	nvme list > /dev/null 2>&1
-
-	rc=$?
+	if ! nvme list | grep -q "${nvmedev}n1"; then
+		echo "NVMeOF device not listed"
+	fi
 
 	nvme disconnect -n "${subsys_name}"
 
@@ -53,9 +53,5 @@ test() {
 	modprobe -r nvme-loop
 	modprobe -r nvmet
 
-	if [ $rc -ne 0 ]; then
-		echo "Test Failed"
-	else
-		echo "Test complete"
-	fi
+	echo "Test complete"
 }
Chaitanya Kulkarni Feb. 20, 2019, 6:19 p.m. UTC | #2 
On 02/20/2019 10:11 AM, Omar Sandoval wrote:
> On Tue, Feb 19, 2019 at 03:28:18PM -0800, Chaitanya Kulkarni wrote:
>> ---
>>   tests/nvme/021     | 15 ++++++---------
>>   tests/nvme/021.out |  1 -
>>   2 files changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/tests/nvme/021 b/tests/nvme/021
>> index 8e9512a..f204a2f 100755
>> --- a/tests/nvme/021
>> +++ b/tests/nvme/021
>> @@ -38,11 +38,13 @@ test() {
>>   	cat "/sys/block/${nvmedev}n1/uuid"
>>   	cat "/sys/block/${nvmedev}n1/wwid"
>>
>> -	nvme list > /dev/null 2>&1
>> -
>> -	rc=$?
>> +	if ! nvme list | grep -q "${nvmedev}n1" > /dev/null 2>&1; then
>> +		echo "Test Failed"
>> +	else
>> +		echo "Test complete"
>> +	fi
>>
>> -	nvme disconnect -n "${subsys_name}"
>> +	nvme disconnect -n "${subsys_name}" > /dev/null 2>&1
>>
>>   	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
>>   	_remove_nvmet_subsystem "${subsys_name}"
>> @@ -53,9 +55,4 @@ test() {
>>   	modprobe -r nvme-loop
>>   	modprobe -r nvmet
>>
>> -	if [ $rc -ne 0 ]; then
>> -		echo "Test Failed"
>> -	else
>> -		echo "Test complete"
>> -	fi
>>   }
>> diff --git a/tests/nvme/021.out b/tests/nvme/021.out
>> index 5fb0601..b6b1a7c 100644
>> --- a/tests/nvme/021.out
>> +++ b/tests/nvme/021.out
>> @@ -1,5 +1,4 @@
>>   Running nvme/021
>>   91fdba0d-f87b-4c25-b80f-db7be1418b9e
>>   uuid.91fdba0d-f87b-4c25-b80f-db7be1418b9e
>> -NQN:blktests-subsystem-1 disconnected 1 controller(s)
>>   Test complete
>> --
>> 1.8.3.1
>>
>
> For all of these, I think it'd be useful to have a more descriptive
> error, like so:
>
> diff --git a/tests/nvme/021 b/tests/nvme/021
> index 8e9512a..0b215c4 100755
> --- a/tests/nvme/021
> +++ b/tests/nvme/021
> @@ -38,9 +38,9 @@ test() {
>   	cat "/sys/block/${nvmedev}n1/uuid"
>   	cat "/sys/block/${nvmedev}n1/wwid"
>
> -	nvme list > /dev/null 2>&1
> -
> -	rc=$?
> +	if ! nvme list | grep -q "${nvmedev}n1"; then
> +		echo "NVMeOF device not listed"
> +	fi
>
>   	nvme disconnect -n "${subsys_name}"
>
> @@ -53,9 +53,5 @@ test() {
>   	modprobe -r nvme-loop
>   	modprobe -r nvmet
>
> -	if [ $rc -ne 0 ]; then
> -		echo "Test Failed"
> -	else
> -		echo "Test complete"
> -	fi
> +	echo "Test complete"
>   }
>

Okay, will send out a new version along with if any more
comments that you have.
Omar Sandoval Feb. 20, 2019, 6:23 p.m. UTC | #3 
On Wed, Feb 20, 2019 at 06:19:58PM +0000, Chaitanya Kulkarni wrote:
> On 02/20/2019 10:11 AM, Omar Sandoval wrote:
> > On Tue, Feb 19, 2019 at 03:28:18PM -0800, Chaitanya Kulkarni wrote:
> >> ---
> >>   tests/nvme/021     | 15 ++++++---------
> >>   tests/nvme/021.out |  1 -
> >>   2 files changed, 6 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/tests/nvme/021 b/tests/nvme/021
> >> index 8e9512a..f204a2f 100755
> >> --- a/tests/nvme/021
> >> +++ b/tests/nvme/021
> >> @@ -38,11 +38,13 @@ test() {
> >>   	cat "/sys/block/${nvmedev}n1/uuid"
> >>   	cat "/sys/block/${nvmedev}n1/wwid"
> >>
> >> -	nvme list > /dev/null 2>&1
> >> -
> >> -	rc=$?
> >> +	if ! nvme list | grep -q "${nvmedev}n1" > /dev/null 2>&1; then
> >> +		echo "Test Failed"
> >> +	else
> >> +		echo "Test complete"
> >> +	fi
> >>
> >> -	nvme disconnect -n "${subsys_name}"
> >> +	nvme disconnect -n "${subsys_name}" > /dev/null 2>&1
> >>
> >>   	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
> >>   	_remove_nvmet_subsystem "${subsys_name}"
> >> @@ -53,9 +55,4 @@ test() {
> >>   	modprobe -r nvme-loop
> >>   	modprobe -r nvmet
> >>
> >> -	if [ $rc -ne 0 ]; then
> >> -		echo "Test Failed"
> >> -	else
> >> -		echo "Test complete"
> >> -	fi
> >>   }
> >> diff --git a/tests/nvme/021.out b/tests/nvme/021.out
> >> index 5fb0601..b6b1a7c 100644
> >> --- a/tests/nvme/021.out
> >> +++ b/tests/nvme/021.out
> >> @@ -1,5 +1,4 @@
> >>   Running nvme/021
> >>   91fdba0d-f87b-4c25-b80f-db7be1418b9e
> >>   uuid.91fdba0d-f87b-4c25-b80f-db7be1418b9e
> >> -NQN:blktests-subsystem-1 disconnected 1 controller(s)
> >>   Test complete
> >> --
> >> 1.8.3.1
> >>
> >
> > For all of these, I think it'd be useful to have a more descriptive
> > error, like so:
> >
> > diff --git a/tests/nvme/021 b/tests/nvme/021
> > index 8e9512a..0b215c4 100755
> > --- a/tests/nvme/021
> > +++ b/tests/nvme/021
> > @@ -38,9 +38,9 @@ test() {
> >   	cat "/sys/block/${nvmedev}n1/uuid"
> >   	cat "/sys/block/${nvmedev}n1/wwid"
> >
> > -	nvme list > /dev/null 2>&1
> > -
> > -	rc=$?
> > +	if ! nvme list | grep -q "${nvmedev}n1"; then
> > +		echo "NVMeOF device not listed"
> > +	fi
> >
> >   	nvme disconnect -n "${subsys_name}"
> >
> > @@ -53,9 +53,5 @@ test() {
> >   	modprobe -r nvme-loop
> >   	modprobe -r nvmet
> >
> > -	if [ $rc -ne 0 ]; then
> > -		echo "Test Failed"
> > -	else
> > -		echo "Test complete"
> > -	fi
> > +	echo "Test complete"
> >   }
> >
> 
> Okay, will send out a new version along with if any more
> comments that you have.

The same comment applies for the rest, but I don't have any other
concerns.
diff mbox series

Patch

diff --git a/tests/nvme/021 b/tests/nvme/021
index 8e9512a..f204a2f 100755
--- a/tests/nvme/021
+++ b/tests/nvme/021
@@ -38,11 +38,13 @@  test() {
 	cat "/sys/block/${nvmedev}n1/uuid"
 	cat "/sys/block/${nvmedev}n1/wwid"
 
-	nvme list > /dev/null 2>&1
-
-	rc=$?
+	if ! nvme list | grep -q "${nvmedev}n1" > /dev/null 2>&1; then
+		echo "Test Failed"
+	else
+		echo "Test complete"
+	fi
 
-	nvme disconnect -n "${subsys_name}"
+	nvme disconnect -n "${subsys_name}" > /dev/null 2>&1
 
 	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
 	_remove_nvmet_subsystem "${subsys_name}"
@@ -53,9 +55,4 @@  test() {
 	modprobe -r nvme-loop
 	modprobe -r nvmet
 
-	if [ $rc -ne 0 ]; then
-		echo "Test Failed"
-	else
-		echo "Test complete"
-	fi
 }
diff --git a/tests/nvme/021.out b/tests/nvme/021.out
index 5fb0601..b6b1a7c 100644
--- a/tests/nvme/021.out
+++ b/tests/nvme/021.out
@@ -1,5 +1,4 @@ 
 Running nvme/021
 91fdba0d-f87b-4c25-b80f-db7be1418b9e
 uuid.91fdba0d-f87b-4c25-b80f-db7be1418b9e
-NQN:blktests-subsystem-1 disconnected 1 controller(s)
 Test complete