Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932786AbbGHHMw (ORCPT ); Wed, 8 Jul 2015 03:12:52 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:34825 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932728AbbGHHMm convert rfc822-to-8bit (ORCPT ); Wed, 8 Jul 2015 03:12:42 -0400 MIME-Version: 1.0 In-Reply-To: <559CB8EC.1090008@wanadoo.fr> References: <1436247543-7326-1-git-send-email-christophe.jaillet@wanadoo.fr> <559C06B4.2010808@gonehiking.org> <559CB8EC.1090008@wanadoo.fr> Date: Wed, 8 Jul 2015 09:12:40 +0200 Message-ID: Subject: Re: [PATCH] [SCSI] FlashPoint: optimize string comparison From: Frans Klaver To: Christophe JAILLET Cc: Khalid Aziz , JBottomley@odin.com, linux-scsi@vger.kernel.org, "linux-kernel@vger.kernel.org" , kernel-janitors@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3112 Lines: 89 On Wed, Jul 8, 2015 at 7:45 AM, Christophe JAILLET wrote: > 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. 80. Parts of the code are indented way too much, resulting in these unreadable lines. I have to say that this entire driver looks like something that (sh|w)ould be in staging right now. Frans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/