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

[Omar Sandoval <osandov@osandov.com>]

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.