From: gaoxiang25@huawei.com (Gao Xiang)
Subject: [PATCH v2] erofs-utils: code for handling incorrect debug level.
Date: Mon, 5 Aug 2019 10:20:21 +0800 [thread overview]
Message-ID: <20190805022019.GA12919@138> (raw)
In-Reply-To: <CAGu0czTR7JzNT_xtH=uez_ZJ3POCPLvK3E-ry=BfW4=HPBwwfg@mail.gmail.com>
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
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> >
prev parent reply other threads:[~2019-08-05 2:20 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190805022019.GA12919@138 \
--to=gaoxiang25@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.