| Message ID | 20220802134628.3464-1-dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: sysfs: use sysfs_streq for string matching | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
David,
We have user API breakage. Are you ok with that?
Before:
$ echo " pid" > ./read_policy
After:
$ echo " pid" > ./read_policy
-bash: echo: write error: Invalid argument
$ echo "pid " > ./read_policy
-bash: echo: write error: Invalid argument
-Anand
On 02/08/2022 21:46, David Sterba wrote:
> We have own string matching helper that duplicates what sysfs_streq
> does, with a slight difference that it skips initial whitespace. So far
> this is used for the drive allocation policy. The initial whitespace
> of written sysfs values should be rather discouraged and we should use a
> standard helper.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/sysfs.c | 21 +--------------------
> 1 file changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 32714ef8e22b..84a992681801 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1150,25 +1150,6 @@ static ssize_t btrfs_generation_show(struct kobject *kobj,
> }
> BTRFS_ATTR(, generation, btrfs_generation_show);
>
> -/*
> - * Look for an exact string @string in @buffer with possible leading or
> - * trailing whitespace
> - */
> -static bool strmatch(const char *buffer, const char *string)
> -{
> - const size_t len = strlen(string);
> -
> - /* Skip leading whitespace */
> - buffer = skip_spaces(buffer);
> -
> - /* Match entire string, check if the rest is whitespace or empty */
> - if (strncmp(string, buffer, len) == 0 &&
> - strlen(skip_spaces(buffer + len)) == 0)
> - return true;
> -
> - return false;
> -}
> -
> static const char * const btrfs_read_policy_name[] = { "pid" };
>
> static ssize_t btrfs_read_policy_show(struct kobject *kobj,
> @@ -1202,7 +1183,7 @@ static ssize_t btrfs_read_policy_store(struct kobject *kobj,
> int i;
>
> for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
> - if (strmatch(buf, btrfs_read_policy_name[i])) {
> + if (sysfs_streq(buf, btrfs_read_policy_name[i])) {
> if (i != fs_devices->read_policy) {
> fs_devices->read_policy = i;
> btrfs_info(fs_devices->fs_info,
On Wed, Aug 03, 2022 at 04:01:23PM +0800, Anand Jain wrote: > > David, > > We have user API breakage. Are you ok with that? So technically it's change of behaviour, OTOH allowing initial/trailing whitespace in the values is not common for other sysfs values. What is accepted is trailing "\n" and that's what the helper provides. I'm not sure why we allowed the extende format for the read policy. It should be safe to do the change now as there are no other values so nobody was writing to the file anyway. > > Before: > $ echo " pid" > ./read_policy > > After: > $ echo " pid" > ./read_policy > -bash: echo: write error: Invalid argument > $ echo "pid " > ./read_policy > -bash: echo: write error: Invalid argument Yeah, do you have a usecase where the " " must be accepted? It may break tests but I'd rather make it slightly more strict and cosistent.
On 03/08/2022 21:15, David Sterba wrote: > On Wed, Aug 03, 2022 at 04:01:23PM +0800, Anand Jain wrote: >> >> David, >> >> We have user API breakage. Are you ok with that? > > So technically it's change of behaviour, OTOH allowing initial/trailing > whitespace in the values is not common for other sysfs values. What is > accepted is trailing "\n" and that's what the helper provides. I'm not > sure why we allowed the extende format for the read policy. It should be > safe to do the change now as there are no other values so nobody was > writing to the file anyway. > >> >> Before: >> $ echo " pid" > ./read_policy >> >> After: >> $ echo " pid" > ./read_policy >> -bash: echo: write error: Invalid argument >> $ echo "pid " > ./read_policy >> -bash: echo: write error: Invalid argument > > Yeah, do you have a usecase where the " " must be accepted? Nope. > It may break > tests but I'd rather make it slightly more strict and cosistent. It is fine with me. The use of sysfs_streq() makes the interface consistent across other sysfs knobs.
On 02/08/2022 21:46, David Sterba wrote: > We have own string matching helper that duplicates what sysfs_streq > does, with a slight difference that it skips initial whitespace. So far > this is used for the drive allocation policy. The initial whitespace > of written sysfs values should be rather discouraged and we should use a > standard helper. > > Signed-off-by: David Sterba <dsterba@suse.com> Reviewed-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/sysfs.c | 21 +-------------------- > 1 file changed, 1 insertion(+), 20 deletions(-) > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > index 32714ef8e22b..84a992681801 100644 > --- a/fs/btrfs/sysfs.c > +++ b/fs/btrfs/sysfs.c > @@ -1150,25 +1150,6 @@ static ssize_t btrfs_generation_show(struct kobject *kobj, > } > BTRFS_ATTR(, generation, btrfs_generation_show); > > -/* > - * Look for an exact string @string in @buffer with possible leading or > - * trailing whitespace > - */ > -static bool strmatch(const char *buffer, const char *string) > -{ > - const size_t len = strlen(string); > - > - /* Skip leading whitespace */ > - buffer = skip_spaces(buffer); > - > - /* Match entire string, check if the rest is whitespace or empty */ > - if (strncmp(string, buffer, len) == 0 && > - strlen(skip_spaces(buffer + len)) == 0) > - return true; > - > - return false; > -} > - > static const char * const btrfs_read_policy_name[] = { "pid" }; > > static ssize_t btrfs_read_policy_show(struct kobject *kobj, > @@ -1202,7 +1183,7 @@ static ssize_t btrfs_read_policy_store(struct kobject *kobj, > int i; > > for (i = 0; i < BTRFS_NR_READ_POLICY; i++) { > - if (strmatch(buf, btrfs_read_policy_name[i])) { > + if (sysfs_streq(buf, btrfs_read_policy_name[i])) { > if (i != fs_devices->read_policy) { > fs_devices->read_policy = i; > btrfs_info(fs_devices->fs_info,
On Tue, Aug 02, 2022 at 03:46:28PM +0200, David Sterba wrote: > We have own string matching helper that duplicates what sysfs_streq > does, with a slight difference that it skips initial whitespace. So far > this is used for the drive allocation policy. The initial whitespace > of written sysfs values should be rather discouraged and we should use a > standard helper. > > Signed-off-by: David Sterba <dsterba@suse.com> Added to misc-next.
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 32714ef8e22b..84a992681801 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -1150,25 +1150,6 @@ static ssize_t btrfs_generation_show(struct kobject *kobj, } BTRFS_ATTR(, generation, btrfs_generation_show); -/* - * Look for an exact string @string in @buffer with possible leading or - * trailing whitespace - */ -static bool strmatch(const char *buffer, const char *string) -{ - const size_t len = strlen(string); - - /* Skip leading whitespace */ - buffer = skip_spaces(buffer); - - /* Match entire string, check if the rest is whitespace or empty */ - if (strncmp(string, buffer, len) == 0 && - strlen(skip_spaces(buffer + len)) == 0) - return true; - - return false; -} - static const char * const btrfs_read_policy_name[] = { "pid" }; static ssize_t btrfs_read_policy_show(struct kobject *kobj, @@ -1202,7 +1183,7 @@ static ssize_t btrfs_read_policy_store(struct kobject *kobj, int i; for (i = 0; i < BTRFS_NR_READ_POLICY; i++) { - if (strmatch(buf, btrfs_read_policy_name[i])) { + if (sysfs_streq(buf, btrfs_read_policy_name[i])) { if (i != fs_devices->read_policy) { fs_devices->read_policy = i; btrfs_info(fs_devices->fs_info,
We have own string matching helper that duplicates what sysfs_streq does, with a slight difference that it skips initial whitespace. So far this is used for the drive allocation policy. The initial whitespace of written sysfs values should be rather discouraged and we should use a standard helper. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/sysfs.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-)