<div dir="ltr"><div>Hi Gao & Guifu,</div><div><br></div><div>In order to eliminate use of a temp variable, I think we can safely do following:</div><div><br></div><div> case 'd':<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> cfg.c_dbg_lvl = 0; // reset the value of debug level to '0'<br> erofs_err("invalid debug level %d\n",<br> atoi(optarg)); <br> return -EINVAL;<br> }<br> break;<br></div><div>I found this version much cleaner. Although it involves extra call to atoi(). but thats trivial.</div><div>Let me know your thoughts.</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Aug 4, 2019 at 9:21 PM Gao Xiang <<a href="mailto:hsiangkao@aol.com">hsiangkao@aol.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Guifu,<br>
<br>
On Sun, Aug 04, 2019 at 11:41:47PM +0800, Li Guifu wrote:<br>
> GAO<br>
> it's a good suggest, you're right<br>
<br>
I know that you don't have outgoing email permission when you are at work.<br>
I think you need to request this permission from your boss again.<br>
<br>
And could you take some spare time off work and review erofs-utils patches?<br>
That is of great help to erofs community. :)<br>
<br>
Thanks,<br>
Gao Xiang<br>
<br>
> <br>
> ?? 2019/8/4 23:25, Gao Xiang ????:<br>
> > Hi Guifu,<br>
> > <br>
> > On Sun, Aug 04, 2019 at 08:57:58PM +0800, Li Guifu wrote:<br>
> > > Shinde and Gao<br>
> > > Does the variable name of debug level use another name ? like d ?<br>
> > > The i is usual a temporary increase or decrease self variable.<br>
> > <br>
> > I think we can use a common varible name in order to avoid<br>
> > too many temporary variables, maybe `i' is not the best<br>
> > naming, but `i' also stands for "a integer".<br>
> > <br>
> > Maybe we can give a better naming? can you name it and<br>
> > submit another patch? I personally don't like define too<br>
> > many used-once variables... How do you think?<br>
> > <br>
> > Thanks,<br>
> > Gao Xiang<br>
> > <br>
> > > <br>
> > > ?? 2019/8/4 19:39, Pratik Shinde ????:<br>
> > > > Hi Gao,<br>
> > > > <br>
> > > > I Agree with your suggestion. Thanks for the additional code change. I<br>
> > > > think thats pretty much our final patch. :)<br>
> > > > <br>
> > > > -Pratik<br>
> > > > <br>
> > > > On Sun, 4 Aug, 2019, 5:01 PM Gao Xiang, <<a href="mailto:hsiangkao@gmx.com" target="_blank">hsiangkao@gmx.com</a>> wrote:<br>
> > > > <br>
> > > > > 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>
> > > > > <br>
> > > > > <a href="https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=dev&id=26097242976cce68e21d8b569dfda63fb68f356c" rel="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">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">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<br>
> > > > > 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>
> > > > > <br>
> > > > <br>
</blockquote></div>