From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 93789C4361B for ; Sat, 5 Dec 2020 23:05:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 61C4D2310E for ; Sat, 5 Dec 2020 23:05:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727039AbgLEXFO (ORCPT ); Sat, 5 Dec 2020 18:05:14 -0500 Received: from bedivere.hansenpartnership.com ([96.44.175.130]:47534 "EHLO bedivere.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725784AbgLEXFN (ORCPT ); Sat, 5 Dec 2020 18:05:13 -0500 Received: from localhost (localhost [127.0.0.1]) by bedivere.hansenpartnership.com (Postfix) with ESMTP id 38BA412803B8; Sat, 5 Dec 2020 15:04:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=hansenpartnership.com; s=20151216; t=1607209473; bh=OHgDzHQ4UVZtoQc0+c8rR/RVimSRVd9Ac21GYuD8peA=; h=Message-ID:Subject:From:To:Date:In-Reply-To:References:From; b=Eq+EiKxG9EjW7mlOdt08xkKc534CrovgkHN6oAqEbvKSK9K9dpjlyf5EtklYPVy+/ CcR6dJjhtW7bsPbWln4KTuKEADODYZYN0jfSIE8i1fnKlnX+hehYP7xyABK/b6eGAo YfZ3FsIX1tSUaR8o92BGXwgbT5DQtboHC7dj+yUc= Received: from bedivere.hansenpartnership.com ([127.0.0.1]) by localhost (bedivere.hansenpartnership.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id FDIwrjWU5y9O; Sat, 5 Dec 2020 15:04:33 -0800 (PST) Received: from jarvis.int.hansenpartnership.com (unknown [IPv6:2601:600:8280:66d1::527]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by bedivere.hansenpartnership.com (Postfix) with ESMTPSA id BDAE31280319; Sat, 5 Dec 2020 15:04:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=hansenpartnership.com; s=20151216; t=1607209473; bh=OHgDzHQ4UVZtoQc0+c8rR/RVimSRVd9Ac21GYuD8peA=; h=Message-ID:Subject:From:To:Date:In-Reply-To:References:From; b=Eq+EiKxG9EjW7mlOdt08xkKc534CrovgkHN6oAqEbvKSK9K9dpjlyf5EtklYPVy+/ CcR6dJjhtW7bsPbWln4KTuKEADODYZYN0jfSIE8i1fnKlnX+hehYP7xyABK/b6eGAo YfZ3FsIX1tSUaR8o92BGXwgbT5DQtboHC7dj+yUc= Message-ID: <8a169362defed5af16be78c5a11f4ff9f58da2a8.camel@HansenPartnership.com> Subject: Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix(). From: James Bottomley To: Ard Biesheuvel Cc: laniel_francis@privacyrequired.com, linux-efi , Linux Kernel Mailing List , Steven Rostedt Date: Sat, 05 Dec 2020 15:04:31 -0800 In-Reply-To: References: <20201204170319.20383-1-laniel_francis@privacyrequired.com> <20201204170319.20383-8-laniel_francis@privacyrequired.com> <3161fc13d69c388b1f51f59c6ecea48dcd0a7856.camel@HansenPartnership.com> <043040d9c092cedcab8bf88b0ec805616d3be44d.camel@HansenPartnership.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.34.4 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 2020-12-05 at 22:20 +0100, Ard Biesheuvel wrote: > On Sat, 5 Dec 2020 at 22:15, James Bottomley > wrote: > > [Rostedt added because this is all his fault] > > On Sat, 2020-12-05 at 21:57 +0100, Ard Biesheuvel wrote: > > > On Sat, 5 Dec 2020 at 21:24, James Bottomley > > > wrote: > > [...] > > > > > So I don't object to using str_has_prefix() in new code in > > > > > this way, but I really don't see the point of touching > > > > > existing code. > > > > > > > > That's your prerogative as a Maintainer ... I was just > > > > explaining what the original author had in mind when > > > > str_has_prefix() was created. > > > > > > > > > > Sure, I fully understand you are not the one proposing these > > > changes. > > > > > > But if the pattern in question is so common, couldn't we go one > > > step further and define something like > > > > > > static inline const u8 *skip_prefix_or_null(const u8 *str, const > > > u8 *prefix) > > > { > > > } > > > > > > which returns a pointer into the original string, or NULL if the > > > prefix is not present. > > > > > > The current patch as proposed has no benefit whatsoever, but even > > > the meaningful alternative you are proposing is not actually an > > > improvement, given that it is not self-explanatory from the name > > > 'str_has_prefix' what it returns, and so the code becomes more > > > difficult to understand. > > > > Ah, so this is the kernel maintainer's syndrome: you see an API > > which isn't quite right for your use case, so you update or change > > it. Then you see other use cases for it and suddenly to you it > > becomes the best thing since sliced bread and with a one ring to > > rule them all mentality you exhort everyone to use this new API > > everywhere. See this comment in the merge commit (495d714ad1400) > > which comes from the merge cover letter: > > > > > - Addition of str_has_prefix() and a few use cases. There > > > currently is a similar function strstart() that is used in > > > a > > > few places, but only returns a bool and not a length. These > > > instances will be removed in the future to use > > > str_has_prefix() instead. > > > > Then you forget about it until someone else acts on your somewhat > > ill considered instruction and actually tries the > > replacement. Once someone takes up your cause, the API shows up in > > dozens of emails and the actual debate about whether or not this is > > such a good API really begins, with the poor person who picked it > > up caught in the crossfire. > > > > As maintainers we really should learn to put the cart before the s/to put/not to put/ > > horse. > > > > I am not disagreeing with any of this, but I simply don't see a point > in merging patches that apparently result in the exact same machine > code to be generated, and don't substantially make the code itself > any better. Well, I think the pattern if (strstarts(option, )) { ... option += strlen(); is a bad one because one day may get updated but not . And if is too far away in the code it might not even show up in the diff, leading to reviewers not noticing either. So I think eliminating the pattern is a definite improvement. Now whether the improvement is enough that we should churn the code base to fix it is another question. James