GAO
it's a good suggest, you're right
在 2019/8/4 23:25, Gao Xiang 写道:
Hi Guifu,On Sun, Aug 04, 2019 at 08:57:58PM +0800, Li Guifu wrote:Shinde and Gao Does the variable name of debug level use another name ? like d ? The i is usual a temporary increase or decrease self variable.I think we can use a common varible name in order to avoid too many temporary variables, maybe `i' is not the best naming, but `i' also stands for "a integer". Maybe we can give a better naming? can you name it and submit another patch? I personally don't like define too many used-once variables... How do you think? Thanks, Gao Xiang?? 2019/8/4 19:39, Pratik Shinde ????:Hi Gao, I Agree with your suggestion. Thanks for the additional code change. I think thats pretty much our final patch. :) -Pratik On Sun, 4 Aug, 2019, 5:01 PM Gao Xiang, <[email protected]> wrote:Hi Pratik, On Sun, Aug 04, 2019 at 04:03:49PM +0530, Pratik Shinde wrote:Hi Gao, I used fprintf here because we are printing this error message in case of invalid 'cfg.c_dbg_lvl'. Hence I thought we cannot rely on erofs_err(). e.g $ mkfs.erofs -d -1 <erofs image> <directory> In this case debug level is '-1' which is invalid.If we try to print the error message using erofs_err() with c_dbg_lvl = -1, it will not print anything.Yes, so c_dbg_lvl should be kept in default level (0) before checking its vaildity I think.While applying the minor fixup, just reset the c_dbg_lvl to 0 , so that erofs_err() will be able to log the error message.Since there could be some messages already printed with erofs_xxx before mkfs_parse_options_cfg(), I think we can use default level (0) before checking its vaildity and switch to the given level after it, as below: case 'd': - cfg.c_dbg_lvl = parse_num_from_str(optarg); + i = atoi(optarg); + if (i < EROFS_MSG_MIN || i > EROFS_MSG_MAX) { + erofs_err("invalid debug level %d", i); + return -EINVAL; + } + cfg.c_dbg_lvl = i; https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=dev&id=26097242976cce68e21d8b569dfda63fb68f356c Do you agree? :) Thanks, Gao Xiang--Pratik. On Sun, Aug 4, 2019 at 3:47 PM Gao Xiang <[email protected]> wrote:On Sun, Aug 04, 2019 at 01:49:43PM +0530, Pratik Shinde wrote:handling the case of incorrect debug level. Added an enumerated type for supported debug levels. Using 'atoi' in place of 'parse_num_from_str'. Signed-off-by: Pratik Shinde <[email protected]> --- include/erofs/print.h | 18 +++++++++++++----- mkfs/main.c | 19 ++++++++----------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/include/erofs/print.h b/include/erofs/print.h index bc0b8d4..296cbbf 100644 --- a/include/erofs/print.h +++ b/include/erofs/print.h @@ -12,6 +12,15 @@ #include "config.h" #include <stdio.h> +enum { + EROFS_MSG_MIN = 0, + EROFS_ERR = 0, + EROFS_WARN = 2, + EROFS_INFO = 3, + EROFS_DBG = 7, + EROFS_MSG_MAX = 9 +}; + #define FUNC_LINE_FMT "%s() Line[%d] " #ifndef pr_fmt @@ -19,7 +28,7 @@ #endif #define erofs_dbg(fmt, ...) do { \ - if (cfg.c_dbg_lvl >= 7) { \ + if (cfg.c_dbg_lvl >= EROFS_DBG) { \ fprintf(stdout, \ pr_fmt(fmt), \ __func__, \ @@ -29,7 +38,7 @@ } while (0) #define erofs_info(fmt, ...) do { \ - if (cfg.c_dbg_lvl >= 3) { \ + if (cfg.c_dbg_lvl >= EROFS_INFO) { \ fprintf(stdout, \ pr_fmt(fmt), \ __func__, \ @@ -40,7 +49,7 @@ } while (0) #define erofs_warn(fmt, ...) do { \ - if (cfg.c_dbg_lvl >= 2) { \ + if (cfg.c_dbg_lvl >= EROFS_WARN) { \ fprintf(stdout, \ pr_fmt(fmt), \ __func__, \ @@ -51,7 +60,7 @@ } while (0) #define erofs_err(fmt, ...) do { \ - if (cfg.c_dbg_lvl >= 0) { \ + if (cfg.c_dbg_lvl >= EROFS_ERR) { \ fprintf(stderr, \ "Err: " pr_fmt(fmt), \ __func__, \ @@ -64,4 +73,3 @@ #endif - diff --git a/mkfs/main.c b/mkfs/main.c index fdb65fd..d915d00 100644 --- a/mkfs/main.c +++ b/mkfs/main.c @@ -30,16 +30,6 @@ static void usage(void) fprintf(stderr, " -EX[,...] X=extended options\n"); } -u64 parse_num_from_str(const char *str) -{ - u64 num = 0; - char *endptr = NULL; - - num = strtoull(str, &endptr, 10); - BUG_ON(num == ULLONG_MAX); - return num; -} - static int parse_extended_opts(const char *opts) { #define MATCH_EXTENTED_OPT(opt, token, keylen) \ @@ -108,7 +98,14 @@ static int mkfs_parse_options_cfg(int argc, char*argv[])break; case 'd': - cfg.c_dbg_lvl = parse_num_from_str(optarg); + cfg.c_dbg_lvl = atoi(optarg); + if (cfg.c_dbg_lvl < EROFS_MSG_MIN + || cfg.c_dbg_lvl > EROFS_MSG_MAX) { + fprintf(stderr, + "invalid debug level %d\n", + cfg.c_dbg_lvl);How about using erofs_err as my previous patch attached? I wonder if there are some specfic reasons to directly use fprintfinstead?I will apply it with this minor fixup (no need to resend again), if you have other considerations, reply me in this thread, thanks. :) Thanks, Gao Xiang+ return -EINVAL; + } break; case 'E': -- 2.9.3
