Re: [PATCH] kdb: Refactor env variables get/set code

From: Sumit Garg
Date: Thu Jan 28 2021 - 00:37:09 EST

Next message: Stephen Zhang: "[PATCH] kdb: kdb_support: Fix debugging information problem"
Previous message: CK Hu: "Re: [PATCH v10 5/9] drm/mediatek: separate gamma module"
In reply to: Daniel Thompson: "Re: [PATCH] kdb: Refactor env variables get/set code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 26 Jan 2021 at 02:13, Daniel Thompson
<daniel.thompson@xxxxxxxxxx> wrote:
>
> On Mon, Jan 25, 2021 at 07:59:45PM +0530, Sumit Garg wrote:
> > Move kdb environment related get/set APIs to a separate file in order
> > to provide an abstraction for environment variables access and hence
> > enhances code readability.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> > ---
> > kernel/debug/kdb/Makefile | 2 +-
> > kernel/debug/kdb/kdb_env.c | 229 +++++++++++++++++++++++++++++++++++++++++
> > kernel/debug/kdb/kdb_main.c | 201 +-----------------------------------
>
> So... a couple of things bother me about this.
>
> 1. This patch mixes together real changes (new functions for example) and
> code motion. That makes is needlessly difficult to review. Code
> motion and changes should always be different patches.
>

Agree.

> 2. I'm rather unconvinced by the premise that removing a continuous
> block of functions from one file to another has a particularly big
> impact on readabiity. The existing environment functions are not
> scattered in many difference places so I think any gain from
> moving them is lost (and then some) by the potential for painful
> merge conflicts, especially if anything needs backporting.
>

There is another aspect here as well regarding users of environment
access functions which are spread across multiple files:
- kdb_main.c
- kdb_io.c
- kdb_support.c

So having a separate file for common functions enhances modularity as
well. But if you still think pains from code motion are more than the
gains then I will drop it.

> I *think* I like the new functions (though I agree with Doug about the
> naming) although it is hard to be entirely sure since they are tangled
> in with the code motion.
>
> Basically I'd rather see the patch without the code motion...
> something that just extracts some of the ad-hoc environmental scans
> into functions and puts the functions near the existing env
> manipulation functions.

Okay.

-Sumit

>
>
> Daniel.
>
> > kernel/debug/kdb/kdb_private.h | 3 +
> > 4 files changed, 235 insertions(+), 200 deletions(-)
> > create mode 100644 kernel/debug/kdb/kdb_env.c
> >
> > diff --git a/kernel/debug/kdb/Makefile b/kernel/debug/kdb/Makefile
> > index efac857..b76aebe 100644
> > --- a/kernel/debug/kdb/Makefile
> > +++ b/kernel/debug/kdb/Makefile
> > @@ -6,7 +6,7 @@
> > # Copyright (c) 2009 Wind River Systems, Inc. All Rights Reserved.
> > #
> >
> > -obj-y := kdb_io.o kdb_main.o kdb_support.o kdb_bt.o gen-kdb_cmds.o kdb_bp.o kdb_debugger.o
> > +obj-y := kdb_io.o kdb_main.o kdb_support.o kdb_bt.o gen-kdb_cmds.o kdb_bp.o kdb_debugger.o kdb_env.o
> > obj-$(CONFIG_KDB_KEYBOARD) += kdb_keyboard.o
> >
> > clean-files := gen-kdb_cmds.c
> > diff --git a/kernel/debug/kdb/kdb_env.c b/kernel/debug/kdb/kdb_env.c
> > new file mode 100644
> > index 0000000..33ab5e6
> > --- /dev/null
> > +++ b/kernel/debug/kdb/kdb_env.c
> > @@ -0,0 +1,229 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Kernel Debugger Architecture Independent Environment Functions
> > + *
> > + * Copyright (c) 1999-2004 Silicon Graphics, Inc. All Rights Reserved.
> > + * Copyright (c) 2009 Wind River Systems, Inc. All Rights Reserved.
> > + * 03/02/13 added new 2.5 kallsyms <xavier.bru@xxxxxxxx>
> > + */
> > +
> > +#include <linux/kdb.h>
> > +#include <linux/string.h>
> > +#include "kdb_private.h"
> > +
> > +/*
> > + * Initial environment. This is all kept static and local to
> > + * this file. We don't want to rely on the memory allocation
> > + * mechanisms in the kernel, so we use a very limited allocate-only
> > + * heap for new and altered environment variables. The entire
> > + * environment is limited to a fixed number of entries (add more
> > + * to __env[] if required) and a fixed amount of heap (add more to
> > + * KDB_ENVBUFSIZE if required).
> > + */
> > +static char *__env[] = {
> > +#if defined(CONFIG_SMP)
> > + "PROMPT=[%d]kdb> ",
> > +#else
> > + "PROMPT=kdb> ",
> > +#endif
> > + "MOREPROMPT=more> ",
> > + "RADIX=16",
> > + "MDCOUNT=8", /* lines of md output */
> > + KDB_PLATFORM_ENV,
> > + "DTABCOUNT=30",
> > + "NOSECT=1",
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > + (char *)0,
> > +};
> > +
> > +static const int __nenv = ARRAY_SIZE(__env);
> > +
> > +/*
> > + * kdbgetenv - This function will return the character string value of
> > + * an environment variable.
> > + * Parameters:
> > + * match A character string representing an environment variable.
> > + * Returns:
> > + * NULL No environment variable matches 'match'
> > + * char* Pointer to string value of environment variable.
> > + */
> > +char *kdbgetenv(const char *match)
> > +{
> > + char **ep = __env;
> > + int matchlen = strlen(match);
> > + int i;
> > +
> > + for (i = 0; i < __nenv; i++) {
> > + char *e = *ep++;
> > +
> > + if (!e)
> > + continue;
> > +
> > + if ((strncmp(match, e, matchlen) == 0)
> > + && ((e[matchlen] == '\0')
> > + || (e[matchlen] == '='))) {
> > + char *cp = strchr(e, '=');
> > +
> > + return cp ? ++cp : "";
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +/*
> > + * kdballocenv - This function is used to allocate bytes for
> > + * environment entries.
> > + * Parameters:
> > + * match A character string representing a numeric value
> > + * Outputs:
> > + * *value the unsigned long representation of the env variable 'match'
> > + * Returns:
> > + * Zero on success, a kdb diagnostic on failure.
> > + * Remarks:
> > + * We use a static environment buffer (envbuffer) to hold the values
> > + * of dynamically generated environment variables (see kdb_set). Buffer
> > + * space once allocated is never free'd, so over time, the amount of space
> > + * (currently 512 bytes) will be exhausted if env variables are changed
> > + * frequently.
> > + */
> > +static char *kdballocenv(size_t bytes)
> > +{
> > +#define KDB_ENVBUFSIZE 512
> > + static char envbuffer[KDB_ENVBUFSIZE];
> > + static int envbufsize;
> > + char *ep = NULL;
> > +
> > + if ((KDB_ENVBUFSIZE - envbufsize) >= bytes) {
> > + ep = &envbuffer[envbufsize];
> > + envbufsize += bytes;
> > + }
> > + return ep;
> > +}
> > +
> > +/*
> > + * kdbgetulenv - This function will return the value of an unsigned
> > + * long-valued environment variable.
> > + * Parameters:
> > + * match A character string representing a numeric value
> > + * Outputs:
> > + * *value the unsigned long represntation of the env variable 'match'
> > + * Returns:
> > + * Zero on success, a kdb diagnostic on failure.
> > + */
> > +int kdbgetulenv(const char *match, unsigned long *value)
> > +{
> > + char *ep;
> > +
> > + ep = kdbgetenv(match);
> > + if (!ep)
> > + return KDB_NOTENV;
> > + if (strlen(ep) == 0)
> > + return KDB_NOENVVALUE;
> > +
> > + *value = simple_strtoul(ep, NULL, 0);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * kdbgetintenv - This function will return the value of an
> > + * integer-valued environment variable.
> > + * Parameters:
> > + * match A character string representing an integer-valued env variable
> > + * Outputs:
> > + * *value the integer representation of the environment variable 'match'
> > + * Returns:
> > + * Zero on success, a kdb diagnostic on failure.
> > + */
> > +int kdbgetintenv(const char *match, int *value)
> > +{
> > + unsigned long val;
> > + int diag;
> > +
> > + diag = kdbgetulenv(match, &val);
> > + if (!diag)
> > + *value = (int) val;
> > + return diag;
> > +}
> > +
> > +/*
> > + * kdb_setenv - Alter an existing environment variable or create a new one.
> > + * Parameters:
> > + * var name of the variable
> > + * val value of the variable
> > + * Returns:
> > + * Zero on success, a kdb diagnostic on failure.
> > + */
> > +int kdb_setenv(const char *var, const char *val)
> > +{
> > + int i;
> > + char *ep;
> > + size_t varlen, vallen;
> > +
> > + varlen = strlen(var);
> > + vallen = strlen(val);
> > + ep = kdballocenv(varlen + vallen + 2);
> > + if (ep == (char *)0)
> > + return KDB_ENVBUFFULL;
> > +
> > + sprintf(ep, "%s=%s", var, val);
> > +
> > + ep[varlen+vallen+1] = '\0';
> > +
> > + for (i = 0; i < __nenv; i++) {
> > + if (__env[i]
> > + && ((strncmp(__env[i], var, varlen) == 0)
> > + && ((__env[i][varlen] == '\0')
> > + || (__env[i][varlen] == '=')))) {
> > + __env[i] = ep;
> > + return 0;
> > + }
> > + }
> > +
> > + /*
> > + * Wasn't existing variable. Fit into slot.
> > + */
> > + for (i = 0; i < __nenv-1; i++) {
> > + if (__env[i] == (char *)0) {
> > + __env[i] = ep;
> > + return 0;
> > + }
> > + }
> > +
> > + return KDB_ENVFULL;
> > +}
> > +
> > +/*
> > + * kdb_prienv - Display the current environment variables.
> > + */
> > +void kdb_prienv(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < __nenv; i++) {
> > + if (__env[i])
> > + kdb_printf("%s\n", __env[i]);
> > + }
> > +}
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index a0989a0..03ba161 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -129,57 +129,6 @@ static kdbmsg_t kdbmsgs[] = {
> >
> > static const int __nkdb_err = ARRAY_SIZE(kdbmsgs);
> >
> > -
> > -/*
> > - * Initial environment. This is all kept static and local to
> > - * this file. We don't want to rely on the memory allocation
> > - * mechanisms in the kernel, so we use a very limited allocate-only
> > - * heap for new and altered environment variables. The entire
> > - * environment is limited to a fixed number of entries (add more
> > - * to __env[] if required) and a fixed amount of heap (add more to
> > - * KDB_ENVBUFSIZE if required).
> > - */
> > -
> > -static char *__env[] = {
> > -#if defined(CONFIG_SMP)
> > - "PROMPT=[%d]kdb> ",
> > -#else
> > - "PROMPT=kdb> ",
> > -#endif
> > - "MOREPROMPT=more> ",
> > - "RADIX=16",
> > - "MDCOUNT=8", /* lines of md output */
> > - KDB_PLATFORM_ENV,
> > - "DTABCOUNT=30",
> > - "NOSECT=1",
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > - (char *)0,
> > -};
> > -
> > -static const int __nenv = ARRAY_SIZE(__env);
> > -
> > struct task_struct *kdb_curr_task(int cpu)
> > {
> > struct task_struct *p = curr_task(cpu);
> > @@ -211,113 +160,6 @@ static inline bool kdb_check_flags(kdb_cmdflags_t flags, int permissions,
> > }
> >
> > /*
> > - * kdbgetenv - This function will return the character string value of
> > - * an environment variable.
> > - * Parameters:
> > - * match A character string representing an environment variable.
> > - * Returns:
> > - * NULL No environment variable matches 'match'
> > - * char* Pointer to string value of environment variable.
> > - */
> > -char *kdbgetenv(const char *match)
> > -{
> > - char **ep = __env;
> > - int matchlen = strlen(match);
> > - int i;
> > -
> > - for (i = 0; i < __nenv; i++) {
> > - char *e = *ep++;
> > -
> > - if (!e)
> > - continue;
> > -
> > - if ((strncmp(match, e, matchlen) == 0)
> > - && ((e[matchlen] == '\0')
> > - || (e[matchlen] == '='))) {
> > - char *cp = strchr(e, '=');
> > - return cp ? ++cp : "";
> > - }
> > - }
> > - return NULL;
> > -}
> > -
> > -/*
> > - * kdballocenv - This function is used to allocate bytes for
> > - * environment entries.
> > - * Parameters:
> > - * match A character string representing a numeric value
> > - * Outputs:
> > - * *value the unsigned long representation of the env variable 'match'
> > - * Returns:
> > - * Zero on success, a kdb diagnostic on failure.
> > - * Remarks:
> > - * We use a static environment buffer (envbuffer) to hold the values
> > - * of dynamically generated environment variables (see kdb_set). Buffer
> > - * space once allocated is never free'd, so over time, the amount of space
> > - * (currently 512 bytes) will be exhausted if env variables are changed
> > - * frequently.
> > - */
> > -static char *kdballocenv(size_t bytes)
> > -{
> > -#define KDB_ENVBUFSIZE 512
> > - static char envbuffer[KDB_ENVBUFSIZE];
> > - static int envbufsize;
> > - char *ep = NULL;
> > -
> > - if ((KDB_ENVBUFSIZE - envbufsize) >= bytes) {
> > - ep = &envbuffer[envbufsize];
> > - envbufsize += bytes;
> > - }
> > - return ep;
> > -}
> > -
> > -/*
> > - * kdbgetulenv - This function will return the value of an unsigned
> > - * long-valued environment variable.
> > - * Parameters:
> > - * match A character string representing a numeric value
> > - * Outputs:
> > - * *value the unsigned long represntation of the env variable 'match'
> > - * Returns:
> > - * Zero on success, a kdb diagnostic on failure.
> > - */
> > -static int kdbgetulenv(const char *match, unsigned long *value)
> > -{
> > - char *ep;
> > -
> > - ep = kdbgetenv(match);
> > - if (!ep)
> > - return KDB_NOTENV;
> > - if (strlen(ep) == 0)
> > - return KDB_NOENVVALUE;
> > -
> > - *value = simple_strtoul(ep, NULL, 0);
> > -
> > - return 0;
> > -}
> > -
> > -/*
> > - * kdbgetintenv - This function will return the value of an
> > - * integer-valued environment variable.
> > - * Parameters:
> > - * match A character string representing an integer-valued env variable
> > - * Outputs:
> > - * *value the integer representation of the environment variable 'match'
> > - * Returns:
> > - * Zero on success, a kdb diagnostic on failure.
> > - */
> > -int kdbgetintenv(const char *match, int *value)
> > -{
> > - unsigned long val;
> > - int diag;
> > -
> > - diag = kdbgetulenv(match, &val);
> > - if (!diag)
> > - *value = (int) val;
> > - return diag;
> > -}
> > -
> > -/*
> > * kdbgetularg - This function will convert a numeric string into an
> > * unsigned long value.
> > * Parameters:
> > @@ -374,10 +216,6 @@ int kdbgetu64arg(const char *arg, u64 *value)
> > */
> > int kdb_set(int argc, const char **argv)
> > {
> > - int i;
> > - char *ep;
> > - size_t varlen, vallen;
> > -
> > /*
> > * we can be invoked two ways:
> > * set var=value argv[1]="var", argv[2]="value"
> > @@ -422,37 +260,7 @@ int kdb_set(int argc, const char **argv)
> > * Tokenizer squashed the '=' sign. argv[1] is variable
> > * name, argv[2] = value.
> > */
> > - varlen = strlen(argv[1]);
> > - vallen = strlen(argv[2]);
> > - ep = kdballocenv(varlen + vallen + 2);
> > - if (ep == (char *)0)
> > - return KDB_ENVBUFFULL;
> > -
> > - sprintf(ep, "%s=%s", argv[1], argv[2]);
> > -
> > - ep[varlen+vallen+1] = '\0';
> > -
> > - for (i = 0; i < __nenv; i++) {
> > - if (__env[i]
> > - && ((strncmp(__env[i], argv[1], varlen) == 0)
> > - && ((__env[i][varlen] == '\0')
> > - || (__env[i][varlen] == '=')))) {
> > - __env[i] = ep;
> > - return 0;
> > - }
> > - }
> > -
> > - /*
> > - * Wasn't existing variable. Fit into slot.
> > - */
> > - for (i = 0; i < __nenv-1; i++) {
> > - if (__env[i] == (char *)0) {
> > - __env[i] = ep;
> > - return 0;
> > - }
> > - }
> > -
> > - return KDB_ENVFULL;
> > + return kdb_setenv(argv[1], argv[2]);
> > }
> >
> > static int kdb_check_regs(void)
> > @@ -2056,12 +1864,7 @@ static int kdb_lsmod(int argc, const char **argv)
> >
> > static int kdb_env(int argc, const char **argv)
> > {
> > - int i;
> > -
> > - for (i = 0; i < __nenv; i++) {
> > - if (__env[i])
> > - kdb_printf("%s\n", __env[i]);
> > - }
> > + kdb_prienv();
> >
> > if (KDB_DEBUG(MASK))
> > kdb_printf("KDBDEBUG=0x%x\n",
> > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
> > index 4b2f79e..ae43a13 100644
> > --- a/kernel/debug/kdb/kdb_private.h
> > +++ b/kernel/debug/kdb/kdb_private.h
> > @@ -105,6 +105,9 @@ extern int kdb_putword(unsigned long, unsigned long, size_t);
> > extern int kdbgetularg(const char *, unsigned long *);
> > extern int kdbgetu64arg(const char *, u64 *);
> > extern char *kdbgetenv(const char *);
> > +extern int kdbgetulenv(const char *match, unsigned long *value);
> > +extern int kdb_setenv(const char *var, const char *val);
> > +extern void kdb_prienv(void);
> > extern int kdbgetaddrarg(int, const char **, int*, unsigned long *,
> > long *, char **);
> > extern int kdbgetsymval(const char *, kdb_symtab_t *);
> > --
> > 2.7.4
> >

Next message: Stephen Zhang: "[PATCH] kdb: kdb_support: Fix debugging information problem"
Previous message: CK Hu: "Re: [PATCH v10 5/9] drm/mediatek: separate gamma module"
In reply to: Daniel Thompson: "Re: [PATCH] kdb: Refactor env variables get/set code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]