[PATCH v2] erofs-utils: code for handling incorrect debug level.
Li Guifu
blucerlee at gmail.com
Sun Aug 4 22:57:58 AEST 2019
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 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
>>>>>
>>>>
>>
>
More information about the Linux-erofs
mailing list