* [PATCH v2] erofs-utils: code for handling incorrect debug level. @ 2019-08-04 8:19 Pratik Shinde 2019-08-04 10:16 ` Gao Xiang 0 siblings, 1 reply; 11+ messages in thread From: Pratik Shinde @ 2019-08-04 8:19 UTC (permalink / raw) 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 <pratikshinde320 at gmail.com> --- 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); + return -EINVAL; + } break; case 'E': -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 8:19 [PATCH v2] erofs-utils: code for handling incorrect debug level Pratik Shinde @ 2019-08-04 10:16 ` Gao Xiang 2019-08-04 10:33 ` Pratik Shinde 0 siblings, 1 reply; 11+ messages in thread From: Gao Xiang @ 2019-08-04 10:16 UTC (permalink / raw) On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> > --- > 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 fprintf instead? 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 10:16 ` Gao Xiang @ 2019-08-04 10:33 ` Pratik Shinde 2019-08-04 11:31 ` Gao Xiang 0 siblings, 1 reply; 11+ messages in thread From: Pratik Shinde @ 2019-08-04 10:33 UTC (permalink / raw) 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. 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. --Pratik. On Sun, Aug 4, 2019@3:47 PM Gao Xiang <hsiangkao@aol.com> wrote: > On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> > > --- > > 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 fprintf instead? > > 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 > > > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20190804/f5d04a4e/attachment.htm> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 10:33 ` Pratik Shinde @ 2019-08-04 11:31 ` Gao Xiang 2019-08-04 11:39 ` Pratik Shinde 0 siblings, 1 reply; 11+ messages in thread From: Gao Xiang @ 2019-08-04 11:31 UTC (permalink / raw) Hi Pratik, On Sun, Aug 04, 2019@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@3:47 PM Gao Xiang <hsiangkao@aol.com> wrote: > > > On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> > > > --- > > > 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 fprintf instead? > > > > 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 > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 11:31 ` Gao Xiang @ 2019-08-04 11:39 ` Pratik Shinde 2019-08-04 12:57 ` Li Guifu 0 siblings, 1 reply; 11+ messages in thread From: Pratik Shinde @ 2019-08-04 11:39 UTC (permalink / raw) 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, <hsiangkao@gmx.com> wrote: > Hi Pratik, > > On Sun, Aug 04, 2019@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@3:47 PM Gao Xiang <hsiangkao@aol.com> wrote: > > > > > On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> > > > > --- > > > > 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 fprintf > instead? > > > > > > 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 > > > > > > > > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20190804/71f12240/attachment.htm> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 11:39 ` Pratik Shinde @ 2019-08-04 12:57 ` Li Guifu 2019-08-04 15:25 ` Gao Xiang 0 siblings, 1 reply; 11+ messages in thread From: Li Guifu @ 2019-08-04 12:57 UTC (permalink / raw) 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. ? 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, <hsiangkao@gmx.com> wrote: > >> Hi Pratik, >> >> On Sun, Aug 04, 2019@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@3:47 PM Gao Xiang <hsiangkao@aol.com> wrote: >>> >>>> On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> >>>>> --- >>>>> 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 fprintf >> instead? >>>> >>>> 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 >>>>> >>>> >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 12:57 ` Li Guifu @ 2019-08-04 15:25 ` Gao Xiang 2019-08-04 15:41 ` Li Guifu 0 siblings, 1 reply; 11+ messages in thread From: Gao Xiang @ 2019-08-04 15:25 UTC (permalink / raw) Hi Guifu, On Sun, Aug 04, 2019@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, <hsiangkao@gmx.com> wrote: > > > > > Hi Pratik, > > > > > > On Sun, Aug 04, 2019@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@3:47 PM Gao Xiang <hsiangkao@aol.com> wrote: > > > > > > > > > On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> > > > > > > --- > > > > > > 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 fprintf > > > instead? > > > > > > > > > > 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 > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 15:25 ` Gao Xiang @ 2019-08-04 15:41 ` Li Guifu 2019-08-04 15:51 ` Gao Xiang 0 siblings, 1 reply; 11+ messages in thread From: Li Guifu @ 2019-08-04 15:41 UTC (permalink / raw) GAO it's a good suggest, you're right ? 2019/8/4 23:25, Gao Xiang ??: > Hi Guifu, > > On Sun, Aug 04, 2019@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, <hsiangkao@gmx.com> wrote: >>> >>>> Hi Pratik, >>>> >>>> On Sun, Aug 04, 2019@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@3:47 PM Gao Xiang <hsiangkao@aol.com> wrote: >>>>> >>>>>> On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> >>>>>>> --- >>>>>>> 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 fprintf >>>> instead? >>>>>> >>>>>> 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 >>>>>>> >>>>>> >>>> >>> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 15:41 ` Li Guifu @ 2019-08-04 15:51 ` Gao Xiang 2019-08-04 19:02 ` Pratik Shinde 0 siblings, 1 reply; 11+ messages in thread From: Gao Xiang @ 2019-08-04 15:51 UTC (permalink / raw) Hi Guifu, On Sun, Aug 04, 2019@11:41:47PM +0800, Li Guifu wrote: > GAO > it's a good suggest, you're right I know that you don't have outgoing email permission when you are at work. I think you need to request this permission from your boss again. And could you take some spare time off work and review erofs-utils patches? That is of great help to erofs community. :) Thanks, Gao Xiang > > ?? 2019/8/4 23:25, Gao Xiang ????: > > Hi Guifu, > > > > On Sun, Aug 04, 2019@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, <hsiangkao@gmx.com> wrote: > > > > > > > > > Hi Pratik, > > > > > > > > > > On Sun, Aug 04, 2019@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@3:47 PM Gao Xiang <hsiangkao@aol.com> wrote: > > > > > > > > > > > > > On Sun, Aug 04, 2019@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 <pratikshinde320 at gmail.com> > > > > > > > > --- > > > > > > > > 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 fprintf > > > > > instead? > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 15:51 ` Gao Xiang @ 2019-08-04 19:02 ` Pratik Shinde 2019-08-05 2:20 ` Gao Xiang 0 siblings, 1 reply; 11+ messages in thread From: Pratik Shinde @ 2019-08-04 19:02 UTC (permalink / raw) Hi Gao & Guifu, In order to eliminate use of a temp variable, I think we can safely do following: case 'd': cfg.c_dbg_lvl = atoi(optarg); if (cfg.c_dbg_lvl < EROFS_MSG_MIN || cfg.c_dbg_lvl > EROFS_MSG_MAX) { cfg.c_dbg_lvl = 0; // reset the value of debug level to '0' erofs_err("invalid debug level %d\n", atoi(optarg)); return -EINVAL; } break; I found this version much cleaner. Although it involves extra call to atoi(). but thats trivial. Let me know your thoughts. On Sun, Aug 4, 2019@9:21 PM Gao Xiang <hsiangkao@aol.com> wrote: > Hi Guifu, > > On Sun, Aug 04, 2019@11:41:47PM +0800, Li Guifu wrote: > > GAO > > it's a good suggest, you're right > > I know that you don't have outgoing email permission when you are at work. > I think you need to request this permission from your boss again. > > And could you take some spare time off work and review erofs-utils patches? > That is of great help to erofs community. :) > > Thanks, > Gao Xiang > > > > > ?? 2019/8/4 23:25, Gao Xiang ????: > > > Hi Guifu, > > > > > > On Sun, Aug 04, 2019@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, <hsiangkao@gmx.com> wrote: > > > > > > > > > > > Hi Pratik, > > > > > > > > > > > > On Sun, Aug 04, 2019@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 <hsiangkao at aol.com> > 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 <pratikshinde320 at gmail.com> > > > > > > > > > --- > > > > > > > > > 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 > fprintf > > > > > > instead? > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20190805/a1eaace9/attachment.htm> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] erofs-utils: code for handling incorrect debug level. 2019-08-04 19:02 ` Pratik Shinde @ 2019-08-05 2:20 ` Gao Xiang 0 siblings, 0 replies; 11+ messages in thread From: Gao Xiang @ 2019-08-05 2:20 UTC (permalink / raw) Hi Pratik, On Mon, Aug 05, 2019@12:32:51AM +0530, Pratik Shinde wrote: > Hi Gao & Guifu, > > In order to eliminate use of a temp variable, I think we can safely do > following: > > case 'd': > cfg.c_dbg_lvl = atoi(optarg); > if (cfg.c_dbg_lvl < EROFS_MSG_MIN > || cfg.c_dbg_lvl > EROFS_MSG_MAX) { > cfg.c_dbg_lvl = 0; // reset the value > of debug level to '0' The default level is set in erofs_init_configure() in config.c by design, and no need to reset again (BTW, the default value could not be 0, maybe 9 or even -1 to for silent mode in the future). Saving the old value before could help, but I think in that way we need to introduce another temp variable to save old default value. I think that is minor, if there are better names here, we can rename `i', or you can submit another patch to introduce another temp variable to replace `i'. Thanks, Gao Xiang > erofs_err("invalid debug level %d\n", > atoi(optarg)); > return -EINVAL; > } > break; > I found this version much cleaner. Although it involves extra call to > atoi(). but thats trivial. > Let me know your thoughts. > > > > On Sun, Aug 4, 2019@9:21 PM Gao Xiang <hsiangkao@aol.com> wrote: > > > Hi Guifu, > > > > On Sun, Aug 04, 2019@11:41:47PM +0800, Li Guifu wrote: > > > GAO > > > it's a good suggest, you're right > > > > I know that you don't have outgoing email permission when you are at work. > > I think you need to request this permission from your boss again. > > > > And could you take some spare time off work and review erofs-utils patches? > > That is of great help to erofs community. :) > > > > Thanks, > > Gao Xiang > > > > > > > > ?? 2019/8/4 23:25, Gao Xiang ????: > > > > Hi Guifu, > > > > > > > > On Sun, Aug 04, 2019@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, <hsiangkao@gmx.com> wrote: > > > > > > > > > > > > > Hi Pratik, > > > > > > > > > > > > > > On Sun, Aug 04, 2019@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 <hsiangkao at aol.com> > > 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 <pratikshinde320 at gmail.com> > > > > > > > > > > --- > > > > > > > > > > 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 > > fprintf > > > > > > > instead? > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-05 2:20 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-04 8:19 [PATCH v2] erofs-utils: code for handling incorrect debug level Pratik Shinde 2019-08-04 10:16 ` Gao Xiang 2019-08-04 10:33 ` Pratik Shinde 2019-08-04 11:31 ` Gao Xiang 2019-08-04 11:39 ` Pratik Shinde 2019-08-04 12:57 ` Li Guifu 2019-08-04 15:25 ` Gao Xiang 2019-08-04 15:41 ` Li Guifu 2019-08-04 15:51 ` Gao Xiang 2019-08-04 19:02 ` Pratik Shinde 2019-08-05 2:20 ` Gao Xiang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.