[PATCH 0/4] Fixup blacklist handling

[PATCH 0/4] Fixup blacklist handling

[Hannes Reinecke]

Hi all,

the SCSI blacklist handling seems to be rather tricky issue;
everytime a fix is included it tends to break other devices.
This patchset attempt to simplify the devlist handling yet again,
but this time implementing the framework for regression testing, too.
So with this patchset we can implement a regression testing in
blocktests and validate any future fixes.

To implement blocktests one simply has to do:

modprobe scsi_debug inq_vendor="Inateck" inq_product="        "
cat /sys/block/sdX/device/blacklist

(which recreates the issue triggering commit b704f70ce2003)
and check if the blacklist value is correct.

As usual, comments and reviews are welcome.

Hannes Reinecke (4):
  scsi: whitespace fixes in scsi_devinfo.c
  scsi_devinfo: fixup string compare
  scsi_debug: allow to specify inquiry vendor and model
  scsi: Export blacklist flags to sysfs

 drivers/scsi/scsi_debug.c   | 25 +++++++++++------
 drivers/scsi/scsi_devinfo.c | 68 +++++++++++++++++++++++++--------------------
 drivers/scsi/scsi_scan.c    |  1 +
 drivers/scsi/scsi_sysfs.c   | 11 ++++++++
 4 files changed, 67 insertions(+), 38 deletions(-)

-- 
1.8.5.6

[PATCH 1/4] scsi: whitespace fixes in scsi_devinfo.c

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 28fea83..776c701 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -304,8 +304,8 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
 			 */
 			to[from_length] = '\0';
 		} else {
-			/* 
-			 * space pad the string if it is short. 
+			/*
+			 * space pad the string if it is short.
 			 */
 			strncpy(&to[from_length], spaces,
 				to_length - from_length);
@@ -325,10 +325,10 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
  * @flags:	if strflags NULL, use this flag value
  *
  * Description:
- * 	Create and add one dev_info entry for @vendor, @model, @strflags or
- * 	@flag. If @compatible, add to the tail of the list, do not space
- * 	pad, and set devinfo->compatible. The scsi_static_device_list entries
- * 	are added with @compatible 1 and @clfags NULL.
+ *	Create and add one dev_info entry for @vendor, @model, @strflags or
+ *	@flag. If @compatible, add to the tail of the list, do not space
+ *	pad, and set devinfo->compatible. The scsi_static_device_list entries
+ *	are added with @compatible 1 and @clfags NULL.
  *
  * Returns: 0 OK, -error on failure.
  **/
@@ -350,11 +350,11 @@ static int scsi_dev_info_list_add(int compatible, char *vendor, char *model,
  * @key:	specify list to use
  *
  * Description:
- * 	Create and add one dev_info entry for @vendor, @model,
- * 	@strflags or @flag in list specified by @key. If @compatible,
- * 	add to the tail of the list, do not space pad, and set
- * 	devinfo->compatible. The scsi_static_device_list entries are
- * 	added with @compatible 1 and @clfags NULL.
+ *	Create and add one dev_info entry for @vendor, @model,
+ *	@strflags or @flag in list specified by @key. If @compatible,
+ *	add to the tail of the list, do not space pad, and set
+ *	devinfo->compatible. The scsi_static_device_list entries are
+ *	added with @compatible 1 and @clfags NULL.
  *
  * Returns: 0 OK, -error on failure.
  **/
@@ -405,7 +405,7 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
  *
  * Description:
  *	Finds the first dev_info entry matching @vendor, @model
- * 	in list specified by @key.
+ *	in list specified by @key.
  *
  * Returns: pointer to matching entry, or ERR_PTR on failure.
  **/
@@ -508,10 +508,10 @@ int scsi_dev_info_list_del_keyed(char *vendor, char *model, int key)
  * @dev_list:	string of device flags to add
  *
  * Description:
- * 	Parse dev_list, and add entries to the scsi_dev_info_list.
- * 	dev_list is of the form "vendor:product:flag,vendor:product:flag".
- * 	dev_list is modified via strsep. Can be called for command line
- * 	addition, for proc or mabye a sysfs interface.
+ *	Parse dev_list, and add entries to the scsi_dev_info_list.
+ *	dev_list is of the form "vendor:product:flag,vendor:product:flag".
+ *	dev_list is modified via strsep. Can be called for command line
+ *	addition, for proc or mabye a sysfs interface.
  *
  * Returns: 0 if OK, -error on failure.
  **/
@@ -701,7 +701,7 @@ static int proc_scsi_devinfo_open(struct inode *inode, struct file *file)
 	return seq_open(file, &scsi_devinfo_seq_ops);
 }
 
-/* 
+/*
  * proc_scsi_dev_info_write - allow additions to scsi_dev_info_list via /proc.
  *
  * Description: Adds a black/white list entry for vendor and model with an
@@ -840,8 +840,8 @@ int scsi_dev_info_remove_list(int key)
  * scsi_init_devinfo - set up the dynamic device list.
  *
  * Description:
- * 	Add command line entries from scsi_dev_flags, then add
- * 	scsi_static_device_list entries to the scsi device info list.
+ *	Add command line entries from scsi_dev_flags, then add
+ *	scsi_static_device_list entries to the scsi device info list.
  */
 int __init scsi_init_devinfo(void)
 {
-- 
1.8.5.6

[PATCH 2/4] scsi_devinfo: fixup string compare

[Hannes Reinecke]

When checking the model and vendor string we need to use the
minimum value of either string, otherwise we'll miss out on
wildcard matches.
And we should avoid matching anything with zero size as this
match will be incorrect.
Without this patch certain Hitachi arrays will not be presenting
VPD pages correctly.

Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 776c701..712ad1f 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -415,7 +415,7 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 	struct scsi_dev_info_list *devinfo;
 	struct scsi_dev_info_list_table *devinfo_table =
 		scsi_devinfo_lookup_by_key(key);
-	size_t vmax, mmax;
+	size_t vmax, mmax, vlen, mlen;
 	const char *vskip, *mskip;
 
 	if (IS_ERR(devinfo_table))
@@ -440,6 +440,8 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 	/* Also skip trailing spaces */
 	while (vmax > 0 && vskip[vmax - 1] == ' ')
 		--vmax;
+	if (!vmax)
+		vskip = NULL;
 
 	mmax = sizeof(devinfo->model);
 	mskip = model;
@@ -449,6 +451,8 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 	}
 	while (mmax > 0 && mskip[mmax - 1] == ' ')
 		--mmax;
+	if (!mmax)
+		mskip = NULL;
 
 	list_for_each_entry(devinfo, &devinfo_table->scsi_dev_info_list,
 			    dev_info_list) {
@@ -456,20 +460,24 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 			/*
 			 * Behave like the older version of get_device_flags.
 			 */
-			if (memcmp(devinfo->vendor, vskip, vmax) ||
-					(vmax < sizeof(devinfo->vendor) &&
-						devinfo->vendor[vmax]))
+			vlen = min(vmax, strlen(devinfo->vendor));
+			if (vskip && vlen &&
+			    memcmp(devinfo->vendor, vskip, vlen))
 				continue;
-			if (memcmp(devinfo->model, mskip, mmax) ||
-					(mmax < sizeof(devinfo->model) &&
-						devinfo->model[mmax]))
+			/* Empty strings never match */
+			mlen = min(mmax, strlen(devinfo->model));
+			if (!vlen && !mlen)
 				continue;
-			return devinfo;
+			if (mskip &&
+			    memcmp(devinfo->model, mskip, mlen))
+				continue;
+			if (vskip || mskip)
+				return devinfo;
 		} else {
 			if (!memcmp(devinfo->vendor, vendor,
-				     sizeof(devinfo->vendor)) &&
-			     !memcmp(devinfo->model, model,
-				      sizeof(devinfo->model)))
+				    sizeof(devinfo->vendor)) &&
+			    !memcmp(devinfo->model, model,
+				    sizeof(devinfo->model)))
 				return devinfo;
 		}
 	}
-- 
1.8.5.6

[PATCH 3/4] scsi_debug: allow to specify inquiry vendor and model

[Hannes Reinecke]

For testing purposes we need to be able to pass in the inquiry
vendor and model.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_debug.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index dc095a2..c8a3f8d 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -953,9 +953,9 @@ static int fetch_to_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr,
 }
 
 
-static const char * inq_vendor_id = "Linux   ";
-static const char * inq_product_id = "scsi_debug      ";
-static const char *inq_product_rev = "0186";	/* version less '.' */
+static char sdebug_inq_vendor_id[9] = "Linux   ";
+static char sdebug_inq_product_id[17] = "scsi_debug      ";
+static char sdebug_inq_product_rev[5] = "0186";	/* version less '.' */
 /* Use some locally assigned NAAs for SAS addresses. */
 static const u64 naa3_comp_a = 0x3222222000000000ULL;
 static const u64 naa3_comp_b = 0x3333333000000000ULL;
@@ -975,8 +975,8 @@ static int inquiry_vpd_83(unsigned char *arr, int port_group_id,
 	arr[0] = 0x2;	/* ASCII */
 	arr[1] = 0x1;
 	arr[2] = 0x0;
-	memcpy(&arr[4], inq_vendor_id, 8);
-	memcpy(&arr[12], inq_product_id, 16);
+	memcpy(&arr[4], sdebug_inq_vendor_id, 8);
+	memcpy(&arr[12], sdebug_inq_product_id, 16);
 	memcpy(&arr[28], dev_id_str, dev_id_str_len);
 	num = 8 + 16 + dev_id_str_len;
 	arr[3] = num;
@@ -1408,9 +1408,9 @@ static int resp_inquiry(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	arr[6] = 0x10; /* claim: MultiP */
 	/* arr[6] |= 0x40; ... claim: EncServ (enclosure services) */
 	arr[7] = 0xa; /* claim: LINKED + CMDQUE */
-	memcpy(&arr[8], inq_vendor_id, 8);
-	memcpy(&arr[16], inq_product_id, 16);
-	memcpy(&arr[32], inq_product_rev, 4);
+	memcpy(&arr[8], sdebug_inq_vendor_id, 8);
+	memcpy(&arr[16], sdebug_inq_product_id, 16);
+	memcpy(&arr[32], sdebug_inq_product_rev, 4);
 	/* version descriptors (2 bytes each) follow */
 	put_unaligned_be16(0xc0, arr + 58);   /* SAM-6 no version claimed */
 	put_unaligned_be16(0x5c0, arr + 60);  /* SPC-5 no version claimed */
@@ -4152,6 +4152,12 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 module_param_named(fake_rw, sdebug_fake_rw, int, S_IRUGO | S_IWUSR);
 module_param_named(guard, sdebug_guard, uint, S_IRUGO);
 module_param_named(host_lock, sdebug_host_lock, bool, S_IRUGO | S_IWUSR);
+module_param_string(inq_vendor, sdebug_inq_vendor_id,
+		    sizeof(sdebug_inq_vendor_id), S_IRUGO|S_IWUSR);
+module_param_string(inq_product, sdebug_inq_product_id,
+		    sizeof(sdebug_inq_product_id), S_IRUGO|S_IWUSR);
+module_param_string(inq_rev, sdebug_inq_product_rev,
+		    sizeof(sdebug_inq_product_rev), S_IRUGO|S_IWUSR);
 module_param_named(lbpu, sdebug_lbpu, int, S_IRUGO);
 module_param_named(lbpws, sdebug_lbpws, int, S_IRUGO);
 module_param_named(lbpws10, sdebug_lbpws10, int, S_IRUGO);
@@ -4203,6 +4209,9 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 MODULE_PARM_DESC(fake_rw, "fake reads/writes instead of copying (def=0)");
 MODULE_PARM_DESC(guard, "protection checksum: 0=crc, 1=ip (def=0)");
 MODULE_PARM_DESC(host_lock, "host_lock is ignored (def=0)");
+MODULE_PARM_DESC(inq_vendor, "SCSI INQUIRY vendor string (def=\"Linux\")");
+MODULE_PARM_DESC(inq_product, "SCSI INQUIRY product string (def=\"scsi_debug\")");
+MODULE_PARM_DESC(inq_rev, "SCSI INQUIRY revision string (def=\"scsi_debug\")");
 MODULE_PARM_DESC(lbpu, "enable LBP, support UNMAP command (def=0)");
 MODULE_PARM_DESC(lbpws, "enable LBP, support WRITE SAME(16) with UNMAP bit (def=0)");
 MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (def=0)");
-- 
1.8.5.6

[PATCH 4/4] scsi: Export blacklist flags to sysfs

[Hannes Reinecke]

Each scsi device is scanned according to the found blacklist flags,
but this information is never presented to sysfs.
This makes it quite hard to figure out if blacklisting worked as
expected.
With this patch we're exporting an additional attribute 'blacklist'
containing the blacklist flags for this device.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_scan.c  |  1 +
 drivers/scsi/scsi_sysfs.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3832ba5..e4e4374 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -984,6 +984,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		scsi_attach_vpd(sdev);
 
 	sdev->max_queue_depth = sdev->queue_depth;
+	sdev->sdev_bflags = *bflags;
 
 	/*
 	 * Ok, the device is now all set up, we can
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5e8ace2..37d436b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -953,6 +953,16 @@ static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
 }
 static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
 
+static ssize_t
+sdev_show_blacklist(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	return snprintf (buf, 20, "0x%x\n", sdev->sdev_bflags);
+}
+static DEVICE_ATTR(blacklist, S_IRUGO, sdev_show_blacklist, NULL);
+
 #ifdef CONFIG_SCSI_DH
 static ssize_t
 sdev_show_dh_state(struct device *dev, struct device_attribute *attr,
@@ -1138,6 +1148,7 @@ static umode_t scsi_sdev_bin_attr_is_visible(struct kobject *kobj,
 	&dev_attr_queue_depth.attr,
 	&dev_attr_queue_type.attr,
 	&dev_attr_wwid.attr,
+	&dev_attr_blacklist.attr,
 #ifdef CONFIG_SCSI_DH
 	&dev_attr_dh_state.attr,
 	&dev_attr_access_state.attr,
-- 
1.8.5.6

Re: [PATCH 2/4] scsi_devinfo: fixup string compare

[Alan Stern]

On Tue, 8 Aug 2017, Hannes Reinecke wrote:

> When checking the model and vendor string we need to use the
> minimum value of either string, otherwise we'll miss out on
> wildcard matches.
> And we should avoid matching anything with zero size as this
> match will be incorrect.
> Without this patch certain Hitachi arrays will not be presenting
> VPD pages correctly.
> 
> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
> Signed-off-by: Hannes Reinecke <hare@suse.com>

This is very questionable.  As described in commit b704f70ce200 ("SCSI: 
fix bug in scsi_dev_info_list matching"), there are devices which have 
vendor or model strings of length 0, and there are entries in the 
global device list like that too.  With this patch, such devices and 
such entries would never match anything.

Also, the idea of using the minimum length of either string doesn't 
sound right.  It would allow a device with string "abc" to match an 
entry in the list with string "abcd", which would be wrong.

Suggestion:

	Allow a device string to match a list entry if the lengths
	are equal or the list entry is shorter (this would be a 
	wildcard match), but not if the list entry is longer.  Here
	the device string length is after leading and trailing blanks
	have been removed.

	As a corollary, allow an empty device string to match an empty 
	list entry but nothing else.

Alan Stern

Re: [PATCH 2/4] scsi_devinfo: fixup string compare

[Hannes Reinecke]

On 08/08/2017 05:25 PM, Alan Stern wrote:
> On Tue, 8 Aug 2017, Hannes Reinecke wrote:
> 
>> When checking the model and vendor string we need to use the
>> minimum value of either string, otherwise we'll miss out on
>> wildcard matches.
>> And we should avoid matching anything with zero size as this
>> match will be incorrect.
>> Without this patch certain Hitachi arrays will not be presenting
>> VPD pages correctly.
>>
>> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
> 
> This is very questionable.  As described in commit b704f70ce200 ("SCSI: 
> fix bug in scsi_dev_info_list matching"), there are devices which have 
> vendor or model strings of length 0, and there are entries in the 
> global device list like that too.  With this patch, such devices and 
> such entries would never match anything.
> 
> Also, the idea of using the minimum length of either string doesn't 
> sound right.  It would allow a device with string "abc" to match an 
> entry in the list with string "abcd", which would be wrong.
> 
> Suggestion:
> 
> 	Allow a device string to match a list entry if the lengths
> 	are equal or the list entry is shorter (this would be a 
> 	wildcard match), but not if the list entry is longer.  Here
> 	the device string length is after leading and trailing blanks
> 	have been removed.
> 
> 	As a corollary, allow an empty device string to match an empty 
> 	list entry but nothing else.
> 
> Alan Stern
> 
Well, so far we haven't encountered any of those devices.
I _have_ validated the patch with the known problematic inquiry strings,
namely

"HITACHI" "OPEN-V"
"Inateck" "       "
"       " "Scanner"
"Promise" "STEX"

and it gives the correct result for all of these.
If you have any inquiry strings for which this does _not_ produce the
correct result please let me know.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/4] scsi_devinfo: fixup string compare

[Alan Stern]

On Tue, 8 Aug 2017, Hannes Reinecke wrote:

> On 08/08/2017 05:25 PM, Alan Stern wrote:
> > On Tue, 8 Aug 2017, Hannes Reinecke wrote:
> > 
> >> When checking the model and vendor string we need to use the
> >> minimum value of either string, otherwise we'll miss out on
> >> wildcard matches.
> >> And we should avoid matching anything with zero size as this
> >> match will be incorrect.
> >> Without this patch certain Hitachi arrays will not be presenting
> >> VPD pages correctly.
> >>
> >> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
> >> Signed-off-by: Hannes Reinecke <hare@suse.com>
> > 
> > This is very questionable.  As described in commit b704f70ce200 ("SCSI: 
> > fix bug in scsi_dev_info_list matching"), there are devices which have 
> > vendor or model strings of length 0, and there are entries in the 
> > global device list like that too.  With this patch, such devices and 
> > such entries would never match anything.
> > 
> > Also, the idea of using the minimum length of either string doesn't 
> > sound right.  It would allow a device with string "abc" to match an 
> > entry in the list with string "abcd", which would be wrong.
> > 
> > Suggestion:
> > 
> > 	Allow a device string to match a list entry if the lengths
> > 	are equal or the list entry is shorter (this would be a 
> > 	wildcard match), but not if the list entry is longer.  Here
> > 	the device string length is after leading and trailing blanks
> > 	have been removed.
> > 
> > 	As a corollary, allow an empty device string to match an empty 
> > 	list entry but nothing else.
> > 
> > Alan Stern
> > 
> Well, so far we haven't encountered any of those devices.

But what if we do in the future?  The code should be correct for 
anything.

> I _have_ validated the patch with the known problematic inquiry strings,
> namely
> 
> "HITACHI" "OPEN-V"
> "Inateck" "       "
> "       " "Scanner"
> "Promise" "STEX"
> 
> and it gives the correct result for all of these.

How can "       " "Scanner" give the correct result?  Oh, I see -- the 
patch description and the code comment both say that empty strings 
never match, but in fact they do -- you allow a match if either the 
vendor or model string is empty, but not if they are both empty.

> If you have any inquiry strings for which this does _not_ produce the
> correct result please let me know.

What about "HITA" "OPEN-V"?  Or "" "OPEN-V"?  Both of these will match
the "HITACHI" "OPEN-" entry, but neither of them should.

What about "ABCD" "Scanner"?  This will match the "        " "Scanner" 
entry, but it shouldn't.

Alan Stern

Re: [PATCH 2/4] scsi_devinfo: fixup string compare

[Bart Van Assche]

On Tue, 2017-08-08 at 12:06 -0400, Alan Stern wrote:
> On Tue, 8 Aug 2017, Hannes Reinecke wrote:
> > On 08/08/2017 05:25 PM, Alan Stern wrote:
> > > On Tue, 8 Aug 2017, Hannes Reinecke wrote:
> > > 
> > > > When checking the model and vendor string we need to use the
> > > > minimum value of either string, otherwise we'll miss out on
> > > > wildcard matches.
> > > > And we should avoid matching anything with zero size as this
> > > > match will be incorrect.
> > > > Without this patch certain Hitachi arrays will not be presenting
> > > > VPD pages correctly.
> > > > 
> > > > Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string
> > > > matching")
> > > > Signed-off-by: Hannes Reinecke <hare@suse.com>
> > > 
> > > This is very questionable.  As described in commit b704f70ce200
> > > ("SCSI: 
> > > fix bug in scsi_dev_info_list matching"), there are devices which
> > > have 
> > > vendor or model strings of length 0, and there are entries in the 
> > > global device list like that too.  With this patch, such devices and 
> > > such entries would never match anything.
> > > 
> > > Also, the idea of using the minimum length of either string doesn't 
> > > sound right.  It would allow a device with string "abc" to match an 
> > > entry in the list with string "abcd", which would be wrong.
> > > 
> > > Suggestion:
> > > 
> > > 	Allow a device string to match a list entry if the lengths
> > > 	are equal or the list entry is shorter (this would be a 
> > > 	wildcard match), but not if the list entry is longer.  Here
> > > 	the device string length is after leading and trailing blanks
> > > 	have been removed.
> > > 
> > > 	As a corollary, allow an empty device string to match an empty 
> > > 	list entry but nothing else.
> > > 
> > > Alan Stern
> > > 
> > 
> > Well, so far we haven't encountered any of those devices.
> 
> But what if we do in the future?  The code should be correct for 
> anything.
> 
> > I _have_ validated the patch with the known problematic inquiry strings,
> > namely
> > 
> > "HITACHI" "OPEN-V"
> > "Inateck" "       "
> > "       " "Scanner"
> > "Promise" "STEX"
> > 
> > and it gives the correct result for all of these.
> 
> How can "       " "Scanner" give the correct result?  Oh, I see -- the 
> patch description and the code comment both say that empty strings 
> never match, but in fact they do -- you allow a match if either the 
> vendor or model string is empty, but not if they are both empty.
> 
> > If you have any inquiry strings for which this does _not_ produce the
> > correct result please let me know.
> 
> What about "HITA" "OPEN-V"?  Or "" "OPEN-V"?  Both of these will match
> the "HITACHI" "OPEN-" entry, but neither of them should.
> 
> What about "ABCD" "Scanner"?  This will match the "        " "Scanner" 
> entry, but it shouldn't.

Hello Hannes,

Is there any case for which you think it is useful to perform a prefix match
on the vendor name? What would break if the code for comparing the vendor
name would be changed into an exact match (after having trimmed trailing
whitespace)?

Thanks,

Bart.

Re: [PATCH 2/4] scsi_devinfo: fixup string compare

[Hannes Reinecke]

On 08/08/2017 06:06 PM, Alan Stern wrote:
> On Tue, 8 Aug 2017, Hannes Reinecke wrote:
> 
>> On 08/08/2017 05:25 PM, Alan Stern wrote:
>>> On Tue, 8 Aug 2017, Hannes Reinecke wrote:
>>>
>>>> When checking the model and vendor string we need to use the
>>>> minimum value of either string, otherwise we'll miss out on
>>>> wildcard matches.
>>>> And we should avoid matching anything with zero size as this
>>>> match will be incorrect.
>>>> Without this patch certain Hitachi arrays will not be presenting
>>>> VPD pages correctly.
>>>>
>>>> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
>>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>>
>>> This is very questionable.  As described in commit b704f70ce200 ("SCSI: 
>>> fix bug in scsi_dev_info_list matching"), there are devices which have 
>>> vendor or model strings of length 0, and there are entries in the 
>>> global device list like that too.  With this patch, such devices and 
>>> such entries would never match anything.
>>>
>>> Also, the idea of using the minimum length of either string doesn't 
>>> sound right.  It would allow a device with string "abc" to match an 
>>> entry in the list with string "abcd", which would be wrong.
>>>
>>> Suggestion:
>>>
>>> 	Allow a device string to match a list entry if the lengths
>>> 	are equal or the list entry is shorter (this would be a 
>>> 	wildcard match), but not if the list entry is longer.  Here
>>> 	the device string length is after leading and trailing blanks
>>> 	have been removed.
>>>
>>> 	As a corollary, allow an empty device string to match an empty 
>>> 	list entry but nothing else.
>>>
>>> Alan Stern
>>>
>> Well, so far we haven't encountered any of those devices.
> 
> But what if we do in the future?  The code should be correct for 
> anything.
> 
>> I _have_ validated the patch with the known problematic inquiry strings,
>> namely
>>
>> "HITACHI" "OPEN-V"
>> "Inateck" "       "
>> "       " "Scanner"
>> "Promise" "STEX"
>>
>> and it gives the correct result for all of these.
> 
> How can "       " "Scanner" give the correct result?  Oh, I see -- the 
> patch description and the code comment both say that empty strings 
> never match, but in fact they do -- you allow a match if either the 
> vendor or model string is empty, but not if they are both empty.
> 
Yeah, should've clarified that.

>> If you have any inquiry strings for which this does _not_ produce the
>> correct result please let me know.
> 
> What about "HITA" "OPEN-V"?  Or "" "OPEN-V"?  Both of these will match
> the "HITACHI" "OPEN-" entry, but neither of them should.
> 
Hmm. "HITA" should not match the "HITACHI"; the search string is always
the full string, and only the devinfo string should be treated as partial.
Some goes for "".

> What about "ABCD" "Scanner"?  This will match the "        " "Scanner" 
> entry, but it shouldn't.
> 
Hmm. Might be. But now we can test this with the scsi_debug module.
I'll be checking and report the results.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/4] scsi_devinfo: fixup string compare

[Hannes Reinecke]

On 08/08/2017 06:11 PM, Bart Van Assche wrote:
> On Tue, 2017-08-08 at 12:06 -0400, Alan Stern wrote:
>> On Tue, 8 Aug 2017, Hannes Reinecke wrote:
>>> On 08/08/2017 05:25 PM, Alan Stern wrote:
>>>> On Tue, 8 Aug 2017, Hannes Reinecke wrote:
>>>>
>>>>> When checking the model and vendor string we need to use the
>>>>> minimum value of either string, otherwise we'll miss out on
>>>>> wildcard matches.
>>>>> And we should avoid matching anything with zero size as this
>>>>> match will be incorrect.
>>>>> Without this patch certain Hitachi arrays will not be presenting
>>>>> VPD pages correctly.
>>>>>
>>>>> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string
>>>>> matching")
>>>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>>>
>>>> This is very questionable.  As described in commit b704f70ce200
>>>> ("SCSI: 
>>>> fix bug in scsi_dev_info_list matching"), there are devices which
>>>> have 
>>>> vendor or model strings of length 0, and there are entries in the 
>>>> global device list like that too.  With this patch, such devices and 
>>>> such entries would never match anything.
>>>>
>>>> Also, the idea of using the minimum length of either string doesn't 
>>>> sound right.  It would allow a device with string "abc" to match an 
>>>> entry in the list with string "abcd", which would be wrong.
>>>>
>>>> Suggestion:
>>>>
>>>> 	Allow a device string to match a list entry if the lengths
>>>> 	are equal or the list entry is shorter (this would be a 
>>>> 	wildcard match), but not if the list entry is longer.  Here
>>>> 	the device string length is after leading and trailing blanks
>>>> 	have been removed.
>>>>
>>>> 	As a corollary, allow an empty device string to match an empty 
>>>> 	list entry but nothing else.
>>>>
>>>> Alan Stern
>>>>
>>>
>>> Well, so far we haven't encountered any of those devices.
>>
>> But what if we do in the future?  The code should be correct for 
>> anything.
>>
>>> I _have_ validated the patch with the known problematic inquiry strings,
>>> namely
>>>
>>> "HITACHI" "OPEN-V"
>>> "Inateck" "       "
>>> "       " "Scanner"
>>> "Promise" "STEX"
>>>
>>> and it gives the correct result for all of these.
>>
>> How can "       " "Scanner" give the correct result?  Oh, I see -- the 
>> patch description and the code comment both say that empty strings 
>> never match, but in fact they do -- you allow a match if either the 
>> vendor or model string is empty, but not if they are both empty.
>>
>>> If you have any inquiry strings for which this does _not_ produce the
>>> correct result please let me know.
>>
>> What about "HITA" "OPEN-V"?  Or "" "OPEN-V"?  Both of these will match
>> the "HITACHI" "OPEN-" entry, but neither of them should.
>>
>> What about "ABCD" "Scanner"?  This will match the "        " "Scanner" 
>> entry, but it shouldn't.
> 
> Hello Hannes,
> 
> Is there any case for which you think it is useful to perform a prefix match
> on the vendor name? What would break if the code for comparing the vendor
> name would be changed into an exact match (after having trimmed trailing
> whitespace)?
> 
Haen't thought about this; but sure, it would simplify things here.
Plus the vendor string typically is short enough so that people don't
feel the need to abbreviate it.

I'll check.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)