Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 26 Apr 2022 15:48:44 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Jagdish Gediya <jvgediya@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        ying.huang@intel.com, dave.hansen@intel.com,
        Andrew Morton <akpm@linux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Richard Fitzgerald <rf@opensource.cirrus.com>
Subject: Re: [PATCH v2] lib/kstrtox.c: Add "false"/"true" support to
 kstrtobool
Message-ID: <Ymf4PNfLcYcf1btz@alley>
References: <20220426064001.14241-1-jvgediya@linux.ibm.com>
 <Yme97bRH3hLcqGfC@alley>
 <YmfDiO6KSRzo8C6e@li-6e1fa1cc-351b-11b2-a85c-b897023bb5f3.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YmfDiO6KSRzo8C6e@li-6e1fa1cc-351b-11b2-a85c-b897023bb5f3.ibm.com>
Precedence: bulk

On Tue 2022-04-26 15:33:52, Jagdish Gediya wrote:
> On Tue, Apr 26, 2022 at 11:39:57AM +0200, Petr Mladek wrote:
> > On Tue 2022-04-26 12:10:01, Jagdish Gediya wrote:
> > > At many places in kernel, It is necessary to convert sysfs input
> > > to corrosponding bool value e.g. "false" or "0" need to be converted
> > > to bool false, "true" or "1" need to be converted to bool true,
> > > places where such conversion is needed currently check the input
> > > string manually, kstrtobool can be utilized at such places but
> > > currently kstrtobool doesn't have support to "false"/"true".
> > >
> > > Add "false"/"true" support to kstrtobool while string conversion
> > > to bool. Modify existing manual sysfs conversions to use kstrtobool().
> > 
> > It looks reasonable. I would just do it slightly other way, see
> > below.
> > 
> > > This patch doesn't have any functionality change.
> > 
> > This is not true. All kstrtobool() callers will react differently
> > on the "true"/"false" input.
> 
> how? Is it related to performance as more characters are compared?
> otherwise semantic wise they will get the expected response, correct?

kstrtobool() returned -EINVAL for "true"/"false" strings before this
patch. It will successfully handle them after this patch.
This is a behavior/functional change that will affect all
existing kstrtobool() callers.

The change makes sense and most likely will not cause any regression.
But are you 100% sure? People do crazy things.

> > > --- a/lib/kstrtox.c
> > > +++ b/lib/kstrtox.c
> > > @@ -377,6 +377,13 @@ int kstrtobool(const char *s, bool *res)
> > >  		}
> > >  		break;
> > >  	default:
> > > +		if (!strncmp(s, "true", 4)) {
> > > +			*res = true;
> > > +			return 0;
> > > +		} else if (!strncmp(s, "false", 5)) {
> > > +			*res = false;
> > > +			return 0;
> > 
> > It should be enough to check the first letter like we do in
> > the other cases. I mean to set true when s[0] is 'T' or 't'
> > and false when s[0] is 'F' or 'f'.
> 
> For "on" and "off", 2 characters are matched, so is it good enough
> to compare only single character for strings "true" and "false"?

Yes, the 1st character is enough to distinguish "true" and "false".
Two characters are needed for "on" and "off" because the 1st
character is the same.

Best Regards,
Petr