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

From: Daniel Thompson
Date: Wed Apr 14 2021 - 08:46:30 EST


On Mon, Feb 08, 2021 at 01:32:22PM +0530, Sumit Garg wrote:
>
> Add two new kdb environment access methods as kdb_setenv() and
> kdb_printenv() in order to abstract out environment access code
> from kdb command functions.
>
> Also, replace (char *)0 with NULL as an initializer for environment
> variables array.

Neither (char *)0 nor NULL are great initializers since, for static
data, these are the default value anyway. Better to just give the array
a explicit dimension and be done.

However... that's a fairly small change and I can fix it up when
applying this.


Daniel.


> Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> Changes in v3:
> - Remove redundant '\0' char assignment.
> - Pick up Doug's review tag.
>
> Changes in v2:
> - Get rid of code motion to separate kdb_env.c file.
> - Replace (char *)0 with NULL.
> - Use kernel-doc style function comments.
> - s/kdb_prienv/kdb_printenv/
>
> kernel/debug/kdb/kdb_main.c | 164 ++++++++++++++++++++++++--------------------
> 1 file changed, 91 insertions(+), 73 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 588062a..69b8f55 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -142,40 +142,40 @@ static const int __nkdb_err = ARRAY_SIZE(kdbmsgs);
>
> static char *__env[] = {
> #if defined(CONFIG_SMP)
> - "PROMPT=[%d]kdb> ",
> + "PROMPT=[%d]kdb> ",
> #else
> - "PROMPT=kdb> ",
> + "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,
> + "MOREPROMPT=more> ",
> + "RADIX=16",
> + "MDCOUNT=8", /* lines of md output */
> + KDB_PLATFORM_ENV,
> + "DTABCOUNT=30",
> + "NOSECT=1",
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> };
>
> static const int __nenv = ARRAY_SIZE(__env);
> @@ -318,6 +318,63 @@ int kdbgetintenv(const char *match, int *value)
> }
>
> /*
> + * kdb_setenv() - Alter an existing environment variable or create a new one.
> + * @var: Name of the variable
> + * @val: Value of the variable
> + *
> + * Return: Zero on success, a kdb diagnostic on failure.
> + */
> +static 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);
> +
> + 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_printenv() - Display the current environment variables.
> + */
> +static void kdb_printenv(void)
> +{
> + int i;
> +
> + for (i = 0; i < __nenv; i++) {
> + if (__env[i])
> + kdb_printf("%s\n", __env[i]);
> + }
> +}
> +
> +/*
> * kdbgetularg - This function will convert a numeric string into an
> * unsigned long value.
> * Parameters:
> @@ -374,10 +431,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 +475,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)
> @@ -2055,12 +2078,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_printenv();
>
> if (KDB_DEBUG(MASK))
> kdb_printf("KDBDEBUG=0x%x\n",
> --
> 2.7.4
>