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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
>>>>
>>>> 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@xxxxxxx			               +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)



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux