2020-05-16 09:31:49

by liwei (GF)

[permalink] [raw]
Subject: [PATCH v2] kdb: Make the internal env 'KDBFLAGS' undefinable

'KDBFLAGS' is an internal variable of kdb, it is combined by 'KDBDEBUG'
and state flags. But the user can define an environment variable named
'KDBFLAGS' too, so let's make it undefinable to avoid confusion.

Signed-off-by: Wei Li <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
v1 -> v2:
- Fix lack of braces.

kernel/debug/kdb/kdb_main.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 4fc43fb17127..75b798340300 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -423,6 +423,8 @@ int kdb_set(int argc, const char **argv)
| (debugflags << KDB_DEBUG_FLAG_SHIFT);

return 0;
+ } else if (strcmp(argv[1], "KDBFLAGS") == 0) {
+ return KDB_NOPERM;
}

/*
--
2.17.1


2020-05-19 11:42:40

by Daniel Thompson

[permalink] [raw]
Subject: Re: [PATCH v2] kdb: Make the internal env 'KDBFLAGS' undefinable

On Sat, May 16, 2020 at 05:26:06PM +0800, Wei Li wrote:
> 'KDBFLAGS' is an internal variable of kdb, it is combined by 'KDBDEBUG'
> and state flags. But the user can define an environment variable named
> 'KDBFLAGS' too, so let's make it undefinable to avoid confusion.
>
> Signed-off-by: Wei Li <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>

I took a quick look at this and find myself thinking of KDBFLAGS as
something of a misfeature.

I think I'd rather get kdb_env to show the value we wrote into
KDBDEBUG.

Sure this means we cannot use KDBDEBUG to look at the least significant
16-bits but I think anyone who is debugging kdb itself should know
enough to use `md4c1 kdb_flags` to read those anyway.


Daniel.



> ---
> v1 -> v2:
> - Fix lack of braces.
>
> kernel/debug/kdb/kdb_main.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 4fc43fb17127..75b798340300 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -423,6 +423,8 @@ int kdb_set(int argc, const char **argv)
> | (debugflags << KDB_DEBUG_FLAG_SHIFT);
>
> return 0;
> + } else if (strcmp(argv[1], "KDBFLAGS") == 0) {
> + return KDB_NOPERM;
> }
>
> /*
> --
> 2.17.1
>

2020-05-19 14:13:29

by liwei (GF)

[permalink] [raw]
Subject: Re: [PATCH v2] kdb: Make the internal env 'KDBFLAGS' undefinable

Hi Daniel,

On 2020/5/19 19:40, Daniel Thompson wrote:
> On Sat, May 16, 2020 at 05:26:06PM +0800, Wei Li wrote:
>> 'KDBFLAGS' is an internal variable of kdb, it is combined by 'KDBDEBUG'
>> and state flags. But the user can define an environment variable named
>> 'KDBFLAGS' too, so let's make it undefinable to avoid confusion.
>>
>> Signed-off-by: Wei Li <[email protected]>
>> Reviewed-by: Douglas Anderson <[email protected]>
>
> I took a quick look at this and find myself thinking of KDBFLAGS as
> something of a misfeature.
>
> I think I'd rather get kdb_env to show the value we wrote into
> KDBDEBUG.
>
> Sure this means we cannot use KDBDEBUG to look at the least significant
> 16-bits but I think anyone who is debugging kdb itself should know
> enough to use `md4c1 kdb_flags` to read those anyway.
>
>

Agree. That will be more clear with no confusion. Currently,
KDBFLAGS will be shown only when KDBDEBUG is set, that is weird too.
So, let's just make it simple.
I can fix it as you suggested in the next post.

Thanks,
Wei