public inbox for linux-btrfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Qu Wenruo <wqu@suse.com>
To: linux-btrfs@vger.kernel.org
Subject: [PATCH v2 0/2] lib/kstrtox: introduce kstrtoull_suffix() helper
Date: Wed, 20 Dec 2023 10:39:59 +1030	[thread overview]
Message-ID: <cover.1703030510.git.wqu@suse.com> (raw)

[CHANGELOG]
v2:
- Use enum bitmap to define suffixes
  This get rid of the upper/lower case problem, and using enum for each
  suffix still provides a somewhat readable result.

- Do proper suffix overflow check
  The previous check for suffix overflow is incorrect at all, would
  still lead to incorrect values.

- Slight refactor for kstrtoull_suffix() code
  

Recently David Sterba <dsterba@suse.cz> exposed some weird behavior of
btrfs sysfs interface, which is utilizing memparse().

One example is using "echo 25e >
/sys/fs/btrfs/<uuid>/devinfo/scrub_speed_max".

The result value is not 0x25e, because there is no "0x" prefix provided.
Nor (25 << 60), as that would overflow.

All these are the caveats of memparse(), which lacks:

- Overflow check
- Reasonable suffix selection
  I know there may be some niche cases for memory layout, but for most
  callers, they just want a kstrtoull() with reasonable suffix
  selection.
  And I don't think exabytes is a reasonable suffix.

So here we introduce a new helper, "kstrtoull_suffix", which has some
the following two abilities added:

- Allow caller to select the suffix they want to enable
  The default list is "KkMmGgTtPp", no unreasonable "Ee" ones.

- Allow suffix parsing with overflow detection.
  The int part detection is already done by _kstrtoull(), and with the
  extra left shift, do the check again before returning the value.

Unfortunately this new helper is still not a drop-in replacement for
memparse():

- No @retptr support
  It's possible to add, but I'm not sure how many call sites need that
  for extra separators, and even if there are, they may want to go
  strsep() and strtok().

- Need to check the failure properly
  Which memparse() callers never seems to check anyway.

Thus the existing memparse() callers need to opt-in.
For now, btrfs usage of memparse() can be easily converted to
kstrtoull_suffix(), in the 2nd patch.

Qu Wenruo (2):
  lib/strtox: introduce kstrtoull_suffix() helper
  btrfs: sysfs: use kstrtoull_suffix() to replace memparse()

 fs/btrfs/sysfs.c        |  20 +++-----
 include/linux/kstrtox.h |  20 ++++++++
 lib/kstrtox.c           | 108 ++++++++++++++++++++++++++++++++++++++--
 3 files changed, 131 insertions(+), 17 deletions(-)

-- 
2.43.0


             reply	other threads:[~2023-12-20  0:10 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-20  0:09 Qu Wenruo [this message]
2023-12-20  0:10 ` [PATCH v2 1/2] lib/strtox: introduce kstrtoull_suffix() helper Qu Wenruo
2023-12-20  5:38   ` David Disseldorp
2023-12-23  9:57     ` Qu Wenruo
2023-12-27  5:38       ` David Disseldorp
2023-12-20 14:17   ` Andy Shevchenko
2023-12-20  0:10 ` [PATCH v2 2/2] btrfs: sysfs: use kstrtoull_suffix() to replace memparse() Qu Wenruo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1703030510.git.wqu@suse.com \
    --to=wqu@suse.com \
    --cc=linux-btrfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox