[PATCH v2] erofs-utils: code for handling incorrect debug level.
Pratik Shinde
pratikshinde320 at gmail.com
Sun Aug 4 21:39:05 AEST 2019
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 at gmx.com> 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 <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/20190804/71f12240/attachment.htm>
More information about the Linux-erofs
mailing list