From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christophe JAILLET Date: Wed, 08 Jul 2015 05:45:16 +0000 Subject: Re: [PATCH] [SCSI] FlashPoint: optimize string comparison Message-Id: <559CB8EC.1090008@wanadoo.fr> List-Id: References: <1436247543-7326-1-git-send-email-christophe.jaillet@wanadoo.fr> <559C06B4.2010808@gonehiking.org> In-Reply-To: <559C06B4.2010808@gonehiking.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Khalid Aziz , Frans Klaver Cc: JBottomley@odin.com, linux-scsi@vger.kernel.org, "linux-kernel@vger.kernel.org" , kernel-janitors@vger.kernel.org Le 07/07/2015 19:04, Khalid Aziz a =C3=A9crit : > On 07/07/2015 02:45 AM, Frans Klaver wrote: >> On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET >> wrote: >>> Stop comparing the strings as soon as we know that they don't match. >>> >>> Signed-off-by: Christophe JAILLET >>> --- >>> 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=20 >>> char p_card, >>> match =3D 1; >>> >>> for (k =3D 0; k < ID_STRING_LENGTH; k++) { >>> - if (p_id_string[k] !=3D=20 >>> FPT_scamInfo[i].id_string[k]) >>> + if (p_id_string[k] !=3D=20 >>> FPT_scamInfo[i].id_string[k]) { >>> match =3D 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=20 > strncmp would indeed be a better way to clean this up. Also, further=20 > down in the same routine: > > if (FPT_scamInfo[match].state =3D ID_UNUSED) { > for (k =3D 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=20 > down. > > Christophe, if you can send a new patch with these clean-ups, that=20 > 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=20 be the way this driver is coded. Should we want to introduce strncmp=20 here, then, as you have noticed, strcpy should be used to. memset could=20 be also used in many places. Then looking elsewhere in the code, many=20 things should, IMHO, also be fixed. (use consistently empty lines=20 before/after code ; use consistently { }...) Another concern to me is "the use of carriage return". In the following=20 examples, things could be much more readable if not limited to 50 chars=20 per line, or so. 1357 FPT_sccbMgrTbl [thisCard][id * 2 + 1358 i ].TarStatus |1359 = SYNC_SUPPORTED ; or 4850 FPT_sccbMgrTbl [p_card] 4851 [currSCCB->TargID]. 4852 TarLUNBusy[0] =3D 1; 4853 if (currSCCB->Sccb_tag) { 4854 if (FPT_BL_Card = [p_card]. 4855 discQCount != =3D 0) 4856 FPT_BL_C= ard 4857 [p_c= ard]. 4858 disc= QCount--; 4859 FPT_BL_Card [p_card]. 4860 discQ_Tbl[cu= rrSCCB-> 4861 Sc= cb_tag] 4862 =3DNULL ; 4863 } else { 4864 if (FPT_BL_Card = [p_card]. 4865 discQCount != =3D 0) 4866 FPT_BL_C= ard 4867 [p_c= ard]. 4868 disc= QCount--; 4869 FPT_BL_Card [p_card]. 4870 discQ_Tbl 4871 [FPT_sccbMgr= Tbl 4872 [p_card][cu= rrSCCB-> 4873 Ta= rgID]. 4874 LunDiscQ_Id= x[0]] 4875 NULL = ; 4876 } I only reported an easy fix based on a semi-automatic detection of code=20 that could be improved. I don't want to make some much more heavy=20 changes that could break the code. Best regards, CJ -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" = in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christophe JAILLET Subject: Re: [PATCH] [SCSI] FlashPoint: optimize string comparison Date: Wed, 08 Jul 2015 07:45:16 +0200 Message-ID: <559CB8EC.1090008@wanadoo.fr> References: <1436247543-7326-1-git-send-email-christophe.jaillet@wanadoo.fr> <559C06B4.2010808@gonehiking.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <559C06B4.2010808@gonehiking.org> Sender: kernel-janitors-owner@vger.kernel.org To: Khalid Aziz , Frans Klaver Cc: JBottomley@odin.com, linux-scsi@vger.kernel.org, "linux-kernel@vger.kernel.org" , kernel-janitors@vger.kernel.org List-Id: linux-scsi@vger.kernel.org Le 07/07/2015 19:04, Khalid Aziz a =C3=A9crit : > On 07/07/2015 02:45 AM, Frans Klaver wrote: >> On Tue, Jul 7, 2015 at 7:39 AM, Christophe JAILLET >> wrote: >>> Stop comparing the strings as soon as we know that they don't match= =2E >>> >>> Signed-off-by: Christophe JAILLET >>> --- >>> 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=20 >>> char p_card, >>> match =3D 1; >>> >>> for (k =3D 0; k < ID_STRING_LENGTH; k++) { >>> - if (p_id_string[k] !=3D=20 >>> FPT_scamInfo[i].id_string[k]) >>> + if (p_id_string[k] !=3D=20 >>> FPT_scamInfo[i].id_string[k]) { >>> match =3D 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= =20 > strncmp would indeed be a better way to clean this up. Also, further=20 > down in the same routine: > > if (FPT_scamInfo[match].state =3D=3D ID_UNUSED) { > for (k =3D 0; k < ID_STRING_LENGTH; k++) { > FPT_scamInfo[match].id_string[k] =3D > p_id_string[k]; > } > > > This should use strncpy instead. There is another similar spot furthe= r=20 > down. > > Christophe, if you can send a new patch with these clean-ups, that=20 > 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= =20 be the way this driver is coded. Should we want to introduce strncmp=20 here, then, as you have noticed, strcpy should be used to. memset could= =20 be also used in many places. Then looking elsewhere in the code, many=20 things should, IMHO, also be fixed. (use consistently empty lines=20 before/after code ; use consistently { }...) Another concern to me is "the use of carriage return". In the following= =20 examples, things could be much more readable if not limited to 50 chars= =20 per line, or so. 1357 FPT_sccbMgrTbl [thisCard][id * 2 + 1358 i ].TarStatus |=3D 1359 SYNC_SUPPORTED ; or 4850 FPT_sccbMgrTbl = [p_card] 4851 [currSCCB->T= argID]. 4852 TarLUNBusy[0= ] =3D 1; 4853 if (currSCCB->Sc= cb_tag) { 4854 if (FPT_= BL_Card [p_card]. 4855 disc= QCount !=3D 0) 4856 = =46PT_BL_Card 4857 = [p_card]. 4858 = discQCount--; 4859 FPT_BL_C= ard [p_card]. 4860 disc= Q_Tbl[currSCCB-> 4861 = Sccb_tag] 4862 =3DN= ULL ; 4863 } else { 4864 if (FPT_= BL_Card [p_card]. 4865 disc= QCount !=3D 0) 4866 = =46PT_BL_Card 4867 = [p_card]. 4868 = discQCount--; 4869 FPT_BL_C= ard [p_card]. 4870 disc= Q_Tbl 4871 [FPT= _sccbMgrTbl 4872 [p_= card][currSCCB-> 4873 = TargID]. 4874 Lun= DiscQ_Idx[0]] =3D 4875 NULL= ; 4876 } I only reported an easy fix based on a semi-automatic detection of code= =20 that could be improved. I don't want to make some much more heavy=20 changes that could break the code. Best regards, CJ -- To unsubscribe from this list: send the line "unsubscribe kernel-janito= rs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932739AbbGHFpf (ORCPT ); Wed, 8 Jul 2015 01:45:35 -0400 Received: from smtp05.smtpout.orange.fr ([80.12.242.127]:43905 "EHLO smtp.smtpout.orange.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932685AbbGHFpY (ORCPT ); Wed, 8 Jul 2015 01:45:24 -0400 X-ME-Helo: [127.0.0.1] X-ME-Date: Wed, 08 Jul 2015 07:45:21 +0200 X-ME-IP: 92.140.216.36 Message-ID: <559CB8EC.1090008@wanadoo.fr> Date: Wed, 08 Jul 2015 07:45:16 +0200 From: Christophe JAILLET User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 Newsgroups: gmane.linux.kernel,gmane.linux.scsi,gmane.linux.kernel.janitors To: Khalid Aziz , Frans Klaver CC: JBottomley@odin.com, linux-scsi@vger.kernel.org, "linux-kernel@vger.kernel.org" , kernel-janitors@vger.kernel.org Subject: Re: [PATCH] [SCSI] FlashPoint: optimize string comparison References: <1436247543-7326-1-git-send-email-christophe.jaillet@wanadoo.fr> <559C06B4.2010808@gonehiking.org> In-Reply-To: <559C06B4.2010808@gonehiking.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Antivirus: avast! (VPS 150707-1, 07/07/2015), Outbound message X-Antivirus-Status: Clean Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 >> wrote: >>> Stop comparing the strings as soon as we know that they don't match. >>> >>> Signed-off-by: Christophe JAILLET >>> --- >>> 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 FPT_sccbMgrTbl [thisCard][id * 2 + 1358 i ].TarStatus |= 1359 SYNC_SUPPORTED ; or 4850 FPT_sccbMgrTbl [p_card] 4851 [currSCCB->TargID]. 4852 TarLUNBusy[0] = 1; 4853 if (currSCCB->Sccb_tag) { 4854 if (FPT_BL_Card [p_card]. 4855 discQCount != 0) 4856 FPT_BL_Card 4857 [p_card]. 4858 discQCount--; 4859 FPT_BL_Card [p_card]. 4860 discQ_Tbl[currSCCB-> 4861 Sccb_tag] 4862 =NULL ; 4863 } else { 4864 if (FPT_BL_Card [p_card]. 4865 discQCount != 0) 4866 FPT_BL_Card 4867 [p_card]. 4868 discQCount--; 4869 FPT_BL_Card [p_card]. 4870 discQ_Tbl 4871 [FPT_sccbMgrTbl 4872 [p_card][currSCCB-> 4873 TargID]. 4874 LunDiscQ_Idx[0]] = 4875 NULL ; 4876 } 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