From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: Khalid Aziz <khalid@gonehiking.org>,
Frans Klaver <fransklaver@gmail.com>
Cc: JBottomley@odin.com, linux-scsi@vger.kernel.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] [SCSI] FlashPoint: optimize string comparison
Date: Wed, 08 Jul 2015 07:45:16 +0200 [thread overview]
Message-ID: <559CB8EC.1090008@wanadoo.fr> (raw)
In-Reply-To: <559C06B4.2010808@gonehiking.org>
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@wanadoo.fr> wrote:
>>> Stop comparing the strings as soon as we know that they don't match.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.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
next prev parent reply other threads:[~2015-07-08 5:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-07 5:39 [PATCH] [SCSI] FlashPoint: optimize string comparison Christophe JAILLET
2015-07-07 8:45 ` Frans Klaver
2015-07-07 17:04 ` Khalid Aziz
2015-07-08 5:45 ` Christophe JAILLET [this message]
2015-07-08 7:12 ` Frans Klaver
2015-07-08 13:59 ` Khalid Aziz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=559CB8EC.1090008@wanadoo.fr \
--to=christophe.jaillet@wanadoo.fr \
--cc=JBottomley@odin.com \
--cc=fransklaver@gmail.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=khalid@gonehiking.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox