<div dir="auto">Hi Gao,<div dir="auto"><br></div><div dir="auto">I Agree with your suggestion. Thanks for the additional code change. I think thats pretty much our final patch. :) </div><div dir="auto"><br></div><div dir="auto">-Pratik</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 4 Aug, 2019, 5:01 PM Gao Xiang, <<a href="mailto:hsiangkao@gmx.com">hsiangkao@gmx.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Pratik,<br>
<br>
On Sun, Aug 04, 2019 at 04:03:49PM +0530, Pratik Shinde wrote:<br>
> Hi Gao,<br>
><br>
> I used fprintf here because we are printing this error message in case of<br>
> invalid 'cfg.c_dbg_lvl'. Hence I thought<br>
> we cannot rely on erofs_err().<br>
> e.g<br>
> $ mkfs.erofs -d -1 <erofs image> <directory><br>
> In this case debug level is '-1' which is invalid.If we try to print the<br>
> error message using erofs_err() with c_dbg_lvl = -1,<br>
> it will not print anything.<br>
<br>
Yes, so c_dbg_lvl should be kept in default level (0) before<br>
checking its vaildity I think.<br>
<br>
> While applying the minor fixup, just reset the c_dbg_lvl to 0 , so that<br>
> erofs_err() will be able to log the error message.<br>
<br>
Since there could be some messages already printed with erofs_xxx before<br>
mkfs_parse_options_cfg(), I think we can use default level (0) before<br>
checking its vaildity and switch to the given level after it, as below:<br>
<br>
case 'd':<br>
- cfg.c_dbg_lvl = parse_num_from_str(optarg);<br>
+ i = atoi(optarg);<br>
+ if (i < EROFS_MSG_MIN || i > EROFS_MSG_MAX) {<br>
+ erofs_err("invalid debug level %d", i);<br>
+ return -EINVAL;<br>
+ }<br>
+ cfg.c_dbg_lvl = i;<br>
<br>
<a href="https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=dev&id=26097242976cce68e21d8b569dfda63fb68f356c" rel="noreferrer noreferrer" target="_blank">https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=dev&id=26097242976cce68e21d8b569dfda63fb68f356c</a><br>
<br>
Do you agree? :)<br>
<br>
Thanks,<br>
Gao Xiang<br>
<br>
><br>
> --Pratik.<br>
><br>
><br>
> On Sun, Aug 4, 2019 at 3:47 PM Gao Xiang <<a href="mailto:hsiangkao@aol.com" target="_blank" rel="noreferrer">hsiangkao@aol.com</a>> wrote:<br>
><br>
> > On Sun, Aug 04, 2019 at 01:49:43PM +0530, Pratik Shinde wrote:<br>
> > > handling the case of incorrect debug level.<br>
> > > Added an enumerated type for supported debug levels.<br>
> > > Using 'atoi' in place of 'parse_num_from_str'.<br>
> > ><br>
> > > Signed-off-by: Pratik Shinde <<a href="mailto:pratikshinde320@gmail.com" target="_blank" rel="noreferrer">pratikshinde320@gmail.com</a>><br>
> > > ---<br>
> > > include/erofs/print.h | 18 +++++++++++++-----<br>
> > > mkfs/main.c | 19 ++++++++-----------<br>
> > > 2 files changed, 21 insertions(+), 16 deletions(-)<br>
> > ><br>
> > > diff --git a/include/erofs/print.h b/include/erofs/print.h<br>
> > > index bc0b8d4..296cbbf 100644<br>
> > > --- a/include/erofs/print.h<br>
> > > +++ b/include/erofs/print.h<br>
> > > @@ -12,6 +12,15 @@<br>
> > > #include "config.h"<br>
> > > #include <stdio.h><br>
> > ><br>
> > > +enum {<br>
> > > + EROFS_MSG_MIN = 0,<br>
> > > + EROFS_ERR = 0,<br>
> > > + EROFS_WARN = 2,<br>
> > > + EROFS_INFO = 3,<br>
> > > + EROFS_DBG = 7,<br>
> > > + EROFS_MSG_MAX = 9<br>
> > > +};<br>
> > > +<br>
> > > #define FUNC_LINE_FMT "%s() Line[%d] "<br>
> > ><br>
> > > #ifndef pr_fmt<br>
> > > @@ -19,7 +28,7 @@<br>
> > > #endif<br>
> > ><br>
> > > #define erofs_dbg(fmt, ...) do { \<br>
> > > - if (cfg.c_dbg_lvl >= 7) { \<br>
> > > + if (cfg.c_dbg_lvl >= EROFS_DBG) { \<br>
> > > fprintf(stdout, \<br>
> > > pr_fmt(fmt), \<br>
> > > __func__, \<br>
> > > @@ -29,7 +38,7 @@<br>
> > > } while (0)<br>
> > ><br>
> > > #define erofs_info(fmt, ...) do { \<br>
> > > - if (cfg.c_dbg_lvl >= 3) { \<br>
> > > + if (cfg.c_dbg_lvl >= EROFS_INFO) { \<br>
> > > fprintf(stdout, \<br>
> > > pr_fmt(fmt), \<br>
> > > __func__, \<br>
> > > @@ -40,7 +49,7 @@<br>
> > > } while (0)<br>
> > ><br>
> > > #define erofs_warn(fmt, ...) do { \<br>
> > > - if (cfg.c_dbg_lvl >= 2) { \<br>
> > > + if (cfg.c_dbg_lvl >= EROFS_WARN) { \<br>
> > > fprintf(stdout, \<br>
> > > pr_fmt(fmt), \<br>
> > > __func__, \<br>
> > > @@ -51,7 +60,7 @@<br>
> > > } while (0)<br>
> > ><br>
> > > #define erofs_err(fmt, ...) do { \<br>
> > > - if (cfg.c_dbg_lvl >= 0) { \<br>
> > > + if (cfg.c_dbg_lvl >= EROFS_ERR) { \<br>
> > > fprintf(stderr, \<br>
> > > "Err: " pr_fmt(fmt), \<br>
> > > __func__, \<br>
> > > @@ -64,4 +73,3 @@<br>
> > ><br>
> > ><br>
> > > #endif<br>
> > > -<br>
> > > diff --git a/mkfs/main.c b/mkfs/main.c<br>
> > > index fdb65fd..d915d00 100644<br>
> > > --- a/mkfs/main.c<br>
> > > +++ b/mkfs/main.c<br>
> > > @@ -30,16 +30,6 @@ static void usage(void)<br>
> > > fprintf(stderr, " -EX[,...] X=extended options\n");<br>
> > > }<br>
> > ><br>
> > > -u64 parse_num_from_str(const char *str)<br>
> > > -{<br>
> > > - u64 num = 0;<br>
> > > - char *endptr = NULL;<br>
> > > -<br>
> > > - num = strtoull(str, &endptr, 10);<br>
> > > - BUG_ON(num == ULLONG_MAX);<br>
> > > - return num;<br>
> > > -}<br>
> > > -<br>
> > > static int parse_extended_opts(const char *opts)<br>
> > > {<br>
> > > #define MATCH_EXTENTED_OPT(opt, token, keylen) \<br>
> > > @@ -108,7 +98,14 @@ static int mkfs_parse_options_cfg(int argc, char<br>
> > *argv[])<br>
> > > break;<br>
> > ><br>
> > > case 'd':<br>
> > > - cfg.c_dbg_lvl = parse_num_from_str(optarg);<br>
> > > + cfg.c_dbg_lvl = atoi(optarg);<br>
> > > + if (cfg.c_dbg_lvl < EROFS_MSG_MIN<br>
> > > + || cfg.c_dbg_lvl > EROFS_MSG_MAX) {<br>
> > > + fprintf(stderr,<br>
> > > + "invalid debug level %d\n",<br>
> > > + cfg.c_dbg_lvl);<br>
> ><br>
> > How about using erofs_err as my previous patch attached?<br>
> > I wonder if there are some specfic reasons to directly use fprintf instead?<br>
> ><br>
> > I will apply it with this minor fixup (no need to resend again), if you<br>
> > have<br>
> > other considerations, reply me in this thread, thanks. :)<br>
> ><br>
> > Thanks,<br>
> > Gao Xiang<br>
> ><br>
> > > + return -EINVAL;<br>
> > > + }<br>
> > > break;<br>
> > ><br>
> > > case 'E':<br>
> > > --<br>
> > > 2.9.3<br>
> > ><br>
> ><br>
</blockquote></div>