Re: [PATCH v2] bcachefs: add support for true/false & yes/no in bool-type options

[Hongbo Li]

On 2024/10/3 19:40, Integral wrote:
Here is my new patch:

Currently, when using bcachefs-tools to set options, bool-type options can
only accept 1 or 0. Add support for accepting true/false and yes/no for
these options.

Signed-off-by: Integral <integ...@murena.io>
---
  fs/bcachefs/opts.c | 28 +++++++++++++++++++++-------
  1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/fs/bcachefs/opts.c b/fs/bcachefs/opts.c
index 232be8a44051..659536d00cb9 100644
--- a/fs/bcachefs/opts.c
+++ b/fs/bcachefs/opts.c
@@ -1,6 +1,7 @@
  // SPDX-License-Identifier: GPL-2.0
  
  #include <linux/kernel.h>
+#include <linux/fs_parser.h>
  
  #include "bcachefs.h"
  #include "compress.h"
@@ -322,6 +323,18 @@ int bch2_opt_validate(const struct bch_option *opt, u64 v, 
struct printbuf *err)
        return 0;
  }
  
+static const struct constant_table bch2_opt_bool_names[] = {
+       { "0",                false },
+       { "false",    false },
+       { "no",               false },
+       { "off",      false },
+       { "1",                true },
+       { "true",     true },
+       { "yes",      true },
+       { "on",               true },
+       { },
+};
+
  int bch2_opt_parse(struct bch_fs *c,
                   const struct bch_option *opt,
                   const char *val, u64 *res,
@@ -332,17 +345,18 @@ int bch2_opt_parse(struct bch_fs *c,
        switch (opt->type) {
        case BCH_OPT_BOOL:
                if (val) {
-                       ret = kstrtou64(val, 10, res);
+                       ret = lookup_constant(bch2_opt_bool_names, val, 
BCH_ERR_option_not_bool);
+                       if (ret != BCH_ERR_option_not_bool) {

I apologize for looking at your patch so late. But this seems something 
wrong. The third parameter should be -BCH_ERR_option_not_bool. And also 
as noted in [1]. The only accepted bool value are: "0", "1", "no", 
"yes", "false", "true". This just like what has been done in 
fs_param_is_bool helper.

[1] Documentation/filesystems/mount_api.rst

Thanks,
Hongbo

+                               *res = ret;
+                       } else {
+                               if (err)
+                                       prt_printf(err, "%s: must be bool", 
opt->attr.name);
+                               return ret;
+                       }
                } else {
-                       ret = 0;
                        *res = 1;
                }
  
-		if (ret < 0 || (*res != 0 && *res != 1)) {
-                       if (err)
-                               prt_printf(err, "%s: must be bool", 
opt->attr.name);
-                       return ret < 0 ? ret : -BCH_ERR_option_not_bool;
-               }
                break;
        case BCH_OPT_UINT:
                if (!val) {