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

From: Omar Sandoval <osandov@osandov.com>
To: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"bvanassche@acm.org" <bvanassche@acm.org>
Subject: Re: [PATCH 1/8] nvme/021: use consistent coding style
Date: Wed, 20 Feb 2019 10:23:59 -0800	[thread overview]
Message-ID: <20190220182359.GD8325@vader> (raw)
In-Reply-To: <SN6PR04MB4527E6227D11DE9066855F55867D0@SN6PR04MB4527.namprd04.prod.outlook.com>

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.