[PATCH 1/2] tests/scsi: add SCSI midlayer test group

[PATCH 1/2] tests/scsi: add SCSI midlayer test group

[Hannes Reinecke]

Add a test group for tests of the SCSI midlayer.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 tests/scsi/group | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 tests/scsi/group

diff --git a/tests/scsi/group b/tests/scsi/group
new file mode 100644
index 0000000..73459a6
--- /dev/null
+++ b/tests/scsi/group
@@ -0,0 +1,26 @@
+#!/bin/bash
+#
+# SCSI regression tests
+#
+# Copyright (C) 2017 Hannes Reinecke, SUSE Linux GmbH
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+#
+# Usually, group_requires() just needs to check that any necessary programs and
+# kernel features are available using the _have_foo helpers. If
+# group_requires() returns non-zero, all tests in this group will be skipped.
+group_requires() {
+	_have_root
+}
-- 
1.8.5.6

[PATCH 2/2] tests/scsi/0001: Regression test for SCSI device blacklisting

[Hannes Reinecke]

SCSI device blacklisting seems to be a tricky subject, with
lots of potential for messing up the selection algorithm.
This adds a test for catching regressions here.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 tests/scsi/001     | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/scsi/001.out | 10 ++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 tests/scsi/001
 create mode 100644 tests/scsi/001.out

diff --git a/tests/scsi/001 b/tests/scsi/001
new file mode 100644
index 0000000..374a458
--- /dev/null
+++ b/tests/scsi/001
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Regression test for scsi device blacklisting
+#
+# Copyright (C) 2017 Hannes Reinecke, SUSE Linux GmbH
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+DESCRIPTION="SCSI device blacklisting"
+
+QUICK=1
+
+CHECK_DMESG=0
+
+requires() {
+    if modinfo scsi_debug | grep -q inq_vendor ; then
+	return 0
+    fi
+    return 1
+}
+
+test() {
+    local inq vendor model host dev blacklist
+
+    echo "Running ${TEST_NAME}"
+
+    for inq in \
+	"                        " \
+	"AAAAAAAABBBBBBBBBBBBBBBB" \
+	"HITACHI OPEN-V          " \
+	"        Scanner         " \
+	"Inateck                 " \
+	"Promise STEX            " \
+	"HITA    OPEN-V          " \
+	"ABCD    Scanner         " ; do
+	vendor=${inq:0:8}
+	model=${inq:8:16}
+	modprobe scsi_debug inq_vendor="$vendor" inq_product="$model"
+	host=$(lsscsi -H | sed -n 's/.\([0-9]*\).*scsi_debug/\1/p')
+	if [ -z "$host" ] ; then
+	    echo "Test failed, scsi_debug could not be loaded"
+	    return 1
+	fi
+	dev=$(lsscsi | grep $host | sed -n 's/.*\/dev\/\(sd[a-z]*\).*/\1/p')
+	if [ -z "$dev" ] ; then
+	    echo "Test failed, SCSI device not found"
+	    rmmod scsi_debug
+	    return 1
+	fi
+	vendor=$(cat /sys/block/$dev/device/vendor)
+	model=$(cat /sys/block/$dev/device/model)
+	blacklist=$(cat /sys/block/$dev/device/blacklist)
+	echo "$vendor $model $blacklist"
+	rmmod scsi_debug
+    done
+    echo "Test complete"
+    return 0
+}
diff --git a/tests/scsi/001.out b/tests/scsi/001.out
new file mode 100644
index 0000000..64db97c
--- /dev/null
+++ b/tests/scsi/001.out
@@ -0,0 +1,10 @@
+Running scsi/001
+                          0x0
+AAAAAAAA BBBBBBBBBBBBBBBB 0x0
+HITACHI  OPEN-V           0x20000
+         Scanner          0x1
+Inateck                   0x0
+Promise  STEX             0x40
+HITA     OPEN-V           0x0
+ABCD     Scanner          0x0
+Test complete
-- 
1.8.5.6

Re: [PATCH 2/2] tests/scsi/0001: Regression test for SCSI device blacklisting

[Bart Van Assche]

On Wed, 2017-08-09 at 12:50 +0200, Hannes Reinecke wrote:
> +requires() {
> +    if modinfo scsi_debug | grep -q inq_vendor ; then
> +	return 0
> +    fi
> +    return 1
> +}

How about changing the above four statements into the following, which is
shorter and more robust?

modinfo scsi_debug | grep -q '^parm:[[:blank:]]*inq_vendor:'

> +	modprobe scsi_debug inq_vendor="$vendor" inq_product="$model"
> +	host=$(lsscsi -H | sed -n 's/.\([0-9]*\).*scsi_debug/\1/p')

Not all systems have lsscsi. How about using /sys/bus/pseudo/drivers/scsi_debug/adapter0/host*
instead?

> +	rmmod scsi_debug

Please check the rmmod return value to catch unload failures due to a
user space process having opened the SCSI device created by scsi_debug.

Thanks,

Bart.

Re: [PATCH 2/2] tests/scsi/0001: Regression test for SCSI device blacklisting

[Omar Sandoval]

On Wed, Aug 09, 2017 at 12:50:06PM +0200, Hannes Reinecke wrote:
> SCSI device blacklisting seems to be a tricky subject, with
> lots of potential for messing up the selection algorithm.
> This adds a test for catching regressions here.

I'm waiting to see how the patches end up before applying this, but I'm
glad to see this test :) A few comments below. I've addressed most of
them and pushed it to https://github.com/osandov/blktests/tree/scsi-blacklist,
but the golden output needs to be updated for v3 of your patches. Could
you modify the patch in my branch and resend? Thanks for adding tests!

> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  tests/scsi/001     | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/scsi/001.out | 10 ++++++++
>  2 files changed, 79 insertions(+)
>  create mode 100644 tests/scsi/001
>  create mode 100644 tests/scsi/001.out
> 
> diff --git a/tests/scsi/001 b/tests/scsi/001
> new file mode 100644
> index 0000000..374a458
> --- /dev/null
> +++ b/tests/scsi/001
> @@ -0,0 +1,69 @@
> +#!/bin/bash
> +#
> +# Regression test for scsi device blacklisting

In my experience with xfstests, the single most useful piece of
information to include for a regression test is the commit or patch it
tests. I fixed this to say 'Regression test for patch "scsi_devinfo:
fixup string compare".'

> +# Copyright (C) 2017 Hannes Reinecke, SUSE Linux GmbH
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +DESCRIPTION="SCSI device blacklisting"
> +
> +QUICK=1
> +
> +CHECK_DMESG=0

I realize that the documentation wasn't clear about what we check for in
dmesg. It's just oopses and warnings and such, so we don't need/want to
disable checking dmesg here. I removed it and improved the
documentation.

> +requires() {
> +    if modinfo scsi_debug | grep -q inq_vendor ; then
> +	return 0
> +    fi
> +    return 1
> +}

There's a helper for this, _have_module_param.

> +test() {
> +    local inq vendor model host dev blacklist
> +
> +    echo "Running ${TEST_NAME}"
> +
> +    for inq in \
> +	"                        " \
> +	"AAAAAAAABBBBBBBBBBBBBBBB" \
> +	"HITACHI OPEN-V          " \
> +	"        Scanner         " \
> +	"Inateck                 " \
> +	"Promise STEX            " \
> +	"HITA    OPEN-V          " \
> +	"ABCD    Scanner         " ; do
> +	vendor=${inq:0:8}
> +	model=${inq:8:16}
> +	modprobe scsi_debug inq_vendor="$vendor" inq_product="$model"
> +	host=$(lsscsi -H | sed -n 's/.\([0-9]*\).*scsi_debug/\1/p')
> +	if [ -z "$host" ] ; then
> +	    echo "Test failed, scsi_debug could not be loaded"
> +	    return 1
> +	fi
> +	dev=$(lsscsi | grep $host | sed -n 's/.*\/dev\/\(sd[a-z]*\).*/\1/p')
> +	if [ -z "$dev" ] ; then
> +	    echo "Test failed, SCSI device not found"
> +	    rmmod scsi_debug
> +	    return 1
> +	fi

The biggest change I made was improving the existing scsi_debug helpers
so you don't have to hardcode this.

> +	vendor=$(cat /sys/block/$dev/device/vendor)
> +	model=$(cat /sys/block/$dev/device/model)
> +	blacklist=$(cat /sys/block/$dev/device/blacklist)
> +	echo "$vendor $model $blacklist"
> +	rmmod scsi_debug
> +    done
> +    echo "Test complete"
> +    return 0
> +}
> diff --git a/tests/scsi/001.out b/tests/scsi/001.out
> new file mode 100644
> index 0000000..64db97c
> --- /dev/null
> +++ b/tests/scsi/001.out
> @@ -0,0 +1,10 @@
> +Running scsi/001
> +                          0x0
> +AAAAAAAA BBBBBBBBBBBBBBBB 0x0
> +HITACHI  OPEN-V           0x20000
> +         Scanner          0x1
> +Inateck                   0x0
> +Promise  STEX             0x40
> +HITA     OPEN-V           0x0
> +ABCD     Scanner          0x0
> +Test complete

As mentioned above, this needs updating for the symbolic constants.

Re: [PATCH 2/2] tests/scsi/0001: Regression test for SCSI device blacklisting

[Omar Sandoval]

On Tue, Aug 15, 2017 at 11:00:49PM -0700, Omar Sandoval wrote:
> On Wed, Aug 09, 2017 at 12:50:06PM +0200, Hannes Reinecke wrote:
> > SCSI device blacklisting seems to be a tricky subject, with
> > lots of potential for messing up the selection algorithm.
> > This adds a test for catching regressions here.
> 
> I'm waiting to see how the patches end up before applying this, but I'm
> glad to see this test :) A few comments below. I've addressed most of
> them and pushed it to https://github.com/osandov/blktests/tree/scsi-blacklist,
> but the golden output needs to be updated for v3 of your patches. Could
> you modify the patch in my branch and resend? Thanks for adding tests!

Hannes, can you resend these tests now that your patches are in? Let me
know if you need any help. Thanks!

Re: [PATCH 1/2] tests/scsi: add SCSI midlayer test group

[Omar Sandoval]

On Wed, Aug 09, 2017 at 12:50:05PM +0200, Hannes Reinecke wrote:
> Add a test group for tests of the SCSI midlayer.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  tests/scsi/group | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 tests/scsi/group
> 
> diff --git a/tests/scsi/group b/tests/scsi/group
> new file mode 100644
> index 0000000..73459a6
> --- /dev/null
> +++ b/tests/scsi/group
> @@ -0,0 +1,26 @@
> +#!/bin/bash
> +#
> +# SCSI regression tests
> +#
> +# Copyright (C) 2017 Hannes Reinecke, SUSE Linux GmbH
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +#
> +# Usually, group_requires() just needs to check that any necessary programs and
> +# kernel features are available using the _have_foo helpers. If
> +# group_requires() returns non-zero, all tests in this group will be skipped.

This comment should have been removed, looks fine otherwise.

> +group_requires() {
> +	_have_root
> +}
> -- 
> 1.8.5.6
>