Re: [PATCH] btrfs: drop unused memparse() parameter

David Sterba
2023-12-07T14:02:19Z

On Thu, Dec 07, 2023 at 10:52:55AM +1100, David Disseldorp wrote:
> On Wed, 6 Dec 2023 19:53:31 +0100, David Sterba wrote:
>
> > On Wed, Dec 06, 2023 at 11:21:43AM +1100, David Disseldorp wrote:
> > > On Tue, 5 Dec 2023 15:22:53 +0100, David Sterba wrote:
> > >
> > > > On Tue, Dec 05, 2023 at 10:13:29PM +1100, David Disseldorp wrote:
> > > > > The @retptr parameter for memparse() is optional.
> > > > > btrfs_devinfo_scrub_speed_max_store() doesn't use it for any input
> > > > > validation, so the parameter can be dropped.
> > > >
> > > > Or should it be used for validation? memparse is also used in
> > > > btrfs_chunk_size_store() that accepts whitespace as trailing characters
> > > > (namely '\n' if the value is from echo).
> > >
> > > It probably should have been used for validation when originally added,
> > > but the current behaviour is now part of the sysfs scrub_speed_max API.
> > > Failing on invalid input would break scripts which do things like
> > > echo clear > /sys/fs/btrfs/UUID/devinfo/1/scrub_speed_max
> >
> > I'm not sure the 'part of the API' is a valid agrument here. It's
> > documented that the value is in bytes and that suffixes can be passed
> > for convenience. How come anybody would use 'clear' in the first place
> > and expect it to work with undefined meaning?
>
> Most people don't read documentation :).

Unfortunatelly true. Here the expected practice is that size values on
input can have a suffix so it should be consistent and understandable
even without reading documentation.

> If there's a willingness to
> accept any fallout from adding the validation then I'm happy to do that.
> Will send a v2.

Yes, I think the simple fix is to add the handling as
btrfs_chunk_size_store() does and this will be the pattern for any
future memparsed values in sysfs.

There's one outlier discard/kbps_limit that takes KiB directly so that's
a plain integer. Eventually discard/max_discard_size can also use memparse.