lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 08 Jul 2015 07:45:16 +0200
From:	Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:	linux-kernel@...r.kernel.org
Cc:	linux-scsi@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] [SCSI] FlashPoint: optimize string comparison

Le 07/07/2015 19:04, Khalid Aziz a écrit :
> On 07/07/2015 02:45 AM, Frans Klaver wrote:
>> On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET
>> <christophe.jaillet@...adoo.fr> wrote:
>>> Stop comparing the strings as soon as we know that they don't match.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>>> ---
>>>   drivers/scsi/FlashPoint.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/FlashPoint.c b/drivers/scsi/FlashPoint.c
>>> index 5c74e4c..24a4d1a 100644
>>> --- a/drivers/scsi/FlashPoint.c
>>> +++ b/drivers/scsi/FlashPoint.c
>>> @@ -6280,8 +6280,10 @@ static unsigned char FPT_scmachid(unsigned 
>>> char p_card,
>>>                  match = 1;
>>>
>>>                  for (k = 0; k < ID_STRING_LENGTH; k++) {
>>> -                       if (p_id_string[k] != 
>>> FPT_scamInfo[i].id_string[k])
>>> +                       if (p_id_string[k] != 
>>> FPT_scamInfo[i].id_string[k]) {
>>>                                  match = 0;
>>> +                               break;
>>> +                       }
>>>                  }
>>>
>>>                  if (match) {
>>
>> Why doesn't this use strncmp?
>>
>> Thanks,
>> Frans
>>
>
> I suspect that is how this code came from Mylex many years ago. Using 
> strncmp would indeed be a better way to clean this up. Also, further 
> down in the same routine:
>
>                 if (FPT_scamInfo[match].state == ID_UNUSED) {
>                         for (k = 0; k < ID_STRING_LENGTH; k++) {
>                                 FPT_scamInfo[match].id_string[k] =
>                                     p_id_string[k];
>                         }
>
>
> This should use strncpy instead. There is another similar spot further 
> down.
>
> Christophe, if you can send a new patch with these clean-ups, that 
> would be great.
>
> Thanks,
> Khalid

Hi,

I'm sorry but I won't propose a new patch for that.

I had the same reaction at first (why not use strncmp?) but it seems to 
be the way this driver is coded. Should we want to introduce strncmp 
here, then, as you have noticed, strcpy should be used to. memset could 
be also used in many places. Then looking elsewhere in the code, many 
things should, IMHO, also be fixed. (use consistently empty lines 
before/after code ; use consistently { }...)

Another concern to me is "the use of carriage return". In the following 
examples, things could be much more readable if not limited to 50 chars 
per line, or so.

1357  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L1357>                                  FPT_sccbMgrTbl  <http://lxr.free-electrons.com/ident?i=FPT_sccbMgrTbl>[thisCard][id  <http://lxr.free-electrons.com/ident?i=id>  * 2 +
1358  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L1358>                                                           i  <http://lxr.free-electrons.com/ident?i=i>].TarStatus |=
1359  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L1359>                                      SYNC_SUPPORTED  <http://lxr.free-electrons.com/ident?i=SYNC_SUPPORTED>;

or

4850  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4850>                                                  FPT_sccbMgrTbl  <http://lxr.free-electrons.com/ident?i=FPT_sccbMgrTbl>[p_card]
4851  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4851>                                                      [currSCCB->TargID].
4852  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4852>                                                      TarLUNBusy[0] = 1;
4853  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4853>                                                  if (currSCCB->Sccb_tag) {
4854  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4854>                                                          if (FPT_BL_Card  <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card].
4855  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4855>                                                              discQCount != 0)
4856  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4856>                                                                  FPT_BL_Card  <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>
4857  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4857>                                                                      [p_card].
4858  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4858>                                                                      discQCount--;
4859  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4859>                                                          FPT_BL_Card  <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card].
4860  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4860>                                                              discQ_Tbl[currSCCB->
4861  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4861>                                                                        Sccb_tag]
4862  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4862>                                                              =NULL  <http://lxr.free-electrons.com/ident?i=NULL>;
4863  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4863>                                                  } else {
4864  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4864>                                                          if (FPT_BL_Card  <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card].
4865  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4865>                                                              discQCount != 0)
4866  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4866>                                                                  FPT_BL_Card  <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>
4867  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4867>                                                                      [p_card].
4868  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4868>                                                                      discQCount--;
4869  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4869>                                                          FPT_BL_Card  <http://lxr.free-electrons.com/ident?i=FPT_BL_Card>[p_card].
4870  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4870>                                                              discQ_Tbl
4871  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4871>                                                              [FPT_sccbMgrTbl  <http://lxr.free-electrons.com/ident?i=FPT_sccbMgrTbl>
4872  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4872>                                                               [p_card][currSCCB->
4873  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4873>                                                                        TargID].
4874  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4874>                                                               LunDiscQ_Idx[0]] =
4875  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4875>                                                              NULL  <http://lxr.free-electrons.com/ident?i=NULL>;
4876  <http://lxr.free-electrons.com/source/drivers/scsi/FlashPoint.c#L4876>                                                  }


I only reported an easy fix based on a semi-automatic detection of code 
that could be improved. I don't want to make some much more heavy 
changes that could break the code.

Best regards,
CJ

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists