Re: [PATCH 2/4] scsi_devinfo: fixup string compare
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
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 ReineckeTeamlead Storage & Networking
h...@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
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
> > >
> > > 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
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
>>>
>>> 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 ReineckeTeamlead Storage & Networking
h...@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
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
> >
> > 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
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
>
> 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 ReineckeTeamlead Storage & Networking
h...@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
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
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
