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

[Sumit Garg <sumit.garg@linaro.org>]

On Mon, 25 Jan 2021 at 22:44, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jan 25, 2021 at 6:30 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > 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@bull.net>
>
> I'm not sure the policy for copying over copyright notices like this,
> but I would have expected them to get copied over from the file you
> got the code from?  These are slightly different.
>

Okay, I will match them exactly.

> > + */
> > +
> > +#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,
>
> In a follow-up patch, I guess these could move from 0 to NULL and
> remove the cast?
>

Sure, I will remove the cast.

>
> > +/*
> > + * 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.
> > + */
>
> In a follow-up patch, the above could be moved to kernel-doc format,
> which we're trying to move to for kgdb when we touch code.
>
> I will leave it up to you about whether the new functions introduced
> in this patch are introduced with the proper kernel doc format or move
> to the right format in the same follow-up patch.
>

Okay, I will move these to kernel-doc format.

>
> > +/*
> > + * kdb_prienv - Display the current environment variables.
> > + */
> > +void kdb_prienv(void)
>
> IMO saving the two characters in the function name isn't worth it,
> especially since this function is called in only one place.  Use
> kdb_printenv()

Sure, I will rename it as kdb_printenv().

-Sumit

>
> -Doug