Received-SPF: pass (google.com: domain of linux-kernel+bounces-11838-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 27 Dec 2023 16:38:32 +1100
From: David Disseldorp <ddiss@samba.org>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org, Andrew Morton
 <akpm@linux-foundation.org>, Christophe JAILLET
 <christophe.jaillet@wanadoo.fr>, Andy Shevchenko
 <andriy.shevchenko@linux.intel.com>, David Laight
 <David.Laight@ACULAB.COM>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] lib/strtox: introduce kstrtoull_suffix() helper
Message-ID: <20231227163832.51e305f7@echidna>
In-Reply-To: <97b85612-16ab-4099-9a8e-426df510d7db@gmx.com>
References: <cover.1703030510.git.wqu@suse.com>
	<e042f40ea5cf7fa8251713d5bb7a485f42c5615b.1703030510.git.wqu@suse.com>
	<20231220163856.274f84a3@echidna>
	<97b85612-16ab-4099-9a8e-426df510d7db@gmx.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sat, 23 Dec 2023 20:27:30 +1030, Qu Wenruo wrote:

> On 2023/12/20 16:08, David Disseldorp wrote:
...
> >> +#define KSTRTOULL_SUFFIX_DEFAULT (SUFFIX_K | SUFFIX_M | SUFFIX_G | SUFFIX_T | SUFFIX_P)  
> >
> > I think it'd be clearer if you dropped this default and had callers
> > explicitly provide the desired suffix mask.  
> 
> Well, that would be long, and would be even longer as the newer naming
> would be MEMPARSE_SUFFIX_*, to be more explicit on what the suffix is for...
> 
> And I really want callers to choose a saner default suffix, thus here
> comes the default one.
> 
> In fact, in my next version, I also found that there are some memparse()
> call sites benefits from the newer suffixes (although won't for the "E"
> one).
> The example is the call site setup_elfcorehdr(). Where the comment only
> mentions KMG, but since memparse() silently added "PE" suffixes, maybe
> on some mainframes we saved some time for one or two lucky admins.

I think it's a sane default, my concern is that _DEFAULT says nothing
about supported units from the caller's perspective. Perhaps
MEMPARSE_SUFFIX_KMGTP or MEMPARSE_UNITS_KMGTP would be clearer.

...
> > With the above changes made, feel free to add
> > Reviewed-by: David Disseldorp <ddiss@suse.de>  
> 
> Thanks for the review, but I'm afraid the newer version would be another
> beast.
> 
> All the ommitted comments would be addressed a in new series.
> >
> > I'll leave the review of patch 2/2 up to others, as I'm still a little
> > worried about sysfs trailing whitespace regressions.  
> 
> That won't be a problem anymore, the new series would keep the old
> @retptr behavior, thus for btrfs part it won't be changed at all.

Sounds good. Will follow up there.

Cheers, David