[RFC PATCH] tools: env: Add an option to have an empty default environment

[RFC PATCH] tools: env: Add an option to have an empty default environment

[Chris Packham]

When building envtools via tools-only_defconfig the builtin defaults
are based on options in the defconfig. For example:

  bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm
  bootdelay=2
  baudrate=115200
  stdin=serial,cros-ec-keyb,usbkbd
  stdout=serial,vidconsole
  stderr=serial,vidconsole
  ethaddr=00:00:11:22:33:44
  eth3addr=00:00:11:22:33:45
  eth5addr=00:00:11:22:33:46
  eth6addr=00:00:11:22:33:47
  ipaddr=1.2.3.4
  bootm_size=0x10000000
  kernel_addr_r=0x1000000
  fdt_addr_r=0xc00000
  ramdisk_addr_r=0x2000000
  scriptaddr=0x1000
  pxefile_addr_r=0x2000

These may or may not be sensible for any particular target. Rather than
trying to have a set of defaults that work for every target add a config
option to make the default environment completely empty.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---

 configs/tools-only_defconfig | 1 +
 env/Kconfig                  | 7 +++++++
 include/env_default.h        | 2 ++
 3 files changed, 10 insertions(+)

diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
index a853abf2b8fc..0ddd0518ef9a 100644
--- a/configs/tools-only_defconfig
+++ b/configs/tools-only_defconfig
@@ -12,6 +12,7 @@ CONFIG_BOOTP_DNS2=y
 CONFIG_OF_CONTROL=y
 CONFIG_OF_HOSTFILE=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
+CONFIG_EMPTY_DEFAULT_ENV=y
 CONFIG_BOOTP_SEND_HOSTNAME=y
 CONFIG_IP_DEFRAG=y
 # CONFIG_ACPIGEN is not set
diff --git a/env/Kconfig b/env/Kconfig
index af4d9cbaa4d8..f7860f01cc16 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -614,6 +614,13 @@ config DEFAULT_ENV_FILE
 	  containing key=value pairs, blank lines and lines beginning
 	  with # are ignored.
 
+config EMPTY_DEFAULT_ENV
+	bool "Create an empty default environment"
+	help
+	  Create an empty default environment. This is intended to be
+	  used when building generic target tools and no sensible
+	  default that can be included.
+
 config ENV_VARS_UBOOT_RUNTIME_CONFIG
 	bool "Add run-time information to the environment"
 	help
diff --git a/include/env_default.h b/include/env_default.h
index 8a0c3057f0aa..859188a8e7c9 100644
--- a/include/env_default.h
+++ b/include/env_default.h
@@ -23,6 +23,7 @@ static char default_environment[] = {
 const uchar default_environment[] = {
 #endif
 #ifndef CONFIG_USE_DEFAULT_ENV_FILE
+#ifndef CONFIG_EMPTY_DEFAULT_ENV
 #ifdef	CONFIG_ENV_CALLBACK_LIST_DEFAULT
 	ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0"
 #endif
@@ -107,6 +108,7 @@ const uchar default_environment[] = {
 #endif
 #ifdef	CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
+#endif
 #endif
 	"\0"
 #else /* CONFIG_USE_DEFAULT_ENV_FILE */
-- 
2.28.0

[RFC PATCH] tools: env: Add an option to have an empty default environment

[Joe Hershberger]

On Wed, Aug 12, 2020 at 8:37 PM Chris Packham <judge.packham@gmail.com> wrote:
>
> When building envtools via tools-only_defconfig the builtin defaults
> are based on options in the defconfig. For example:
>
>   bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm
>   bootdelay=2
>   baudrate=115200
>   stdin=serial,cros-ec-keyb,usbkbd
>   stdout=serial,vidconsole
>   stderr=serial,vidconsole
>   ethaddr=00:00:11:22:33:44
>   eth3addr=00:00:11:22:33:45
>   eth5addr=00:00:11:22:33:46
>   eth6addr=00:00:11:22:33:47
>   ipaddr=1.2.3.4
>   bootm_size=0x10000000
>   kernel_addr_r=0x1000000
>   fdt_addr_r=0xc00000
>   ramdisk_addr_r=0x2000000
>   scriptaddr=0x1000
>   pxefile_addr_r=0x2000
>
> These may or may not be sensible for any particular target. Rather than
> trying to have a set of defaults that work for every target add a config
> option to make the default environment completely empty.
>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[RFC PATCH] tools: env: Add an option to have an empty default environment

[Wolfgang Denk]

Dear Chris Packham,

In message <20200813013727.8186-1-judge.packham@gmail.com> you wrote:
> When building envtools via tools-only_defconfig the builtin defaults
> are based on options in the defconfig. For example:
>
>   bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm
>   bootdelay=2
>   baudrate=115200
>   stdin=serial,cros-ec-keyb,usbkbd
>   stdout=serial,vidconsole
>   stderr=serial,vidconsole
>   ethaddr=00:00:11:22:33:44
>   eth3addr=00:00:11:22:33:45
>   eth5addr=00:00:11:22:33:46
>   eth6addr=00:00:11:22:33:47

Having any specific MAC addresses set in the default environment has
always been a strict No-Go as it is just begging for trouble.

If this was a real example, that code should be fixed!!

> These may or may not be sensible for any particular target. Rather than
> trying to have a set of defaults that work for every target add a config
> option to make the default environment completely empty.

An empty default environment is even worse, as you are missing vital
settings like baudrate, bootdelay etc.  There are good chances to
brick your board with such settings.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The word "fit", as I understand it, means "appropriate to a purpose",
and I would say the body of the Dean is supremely appropriate to  the
purpose of sitting around all day and eating big heavy meals.
                                 - Terry Pratchett, _Moving Pictures_

[RFC PATCH] tools: env: Add an option to have an empty default environment

[Stefano Babic]

Hi Chris,

On 13.08.20 03:37, Chris Packham wrote:
> When building envtools via tools-only_defconfig the builtin defaults
> are based on options in the defconfig. For example:
> 
>   bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm
>   bootdelay=2
>   baudrate=115200
>   stdin=serial,cros-ec-keyb,usbkbd
>   stdout=serial,vidconsole
>   stderr=serial,vidconsole
>   ethaddr=00:00:11:22:33:44
>   eth3addr=00:00:11:22:33:45
>   eth5addr=00:00:11:22:33:46
>   eth6addr=00:00:11:22:33:47
>   ipaddr=1.2.3.4
>   bootm_size=0x10000000
>   kernel_addr_r=0x1000000
>   fdt_addr_r=0xc00000
>   ramdisk_addr_r=0x2000000
>   scriptaddr=0x1000
>   pxefile_addr_r=0x2000
> 
> These may or may not be sensible for any particular target. Rather than
> trying to have a set of defaults that work for every target add a config
> option to make the default environment completely empty.
> 
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---

Which is the added value compared to use CONFIG_USE_DEFAULT_ENV_FILE=y
and adding your file with no variables ?

Best regards,
Stefano

> 
>  configs/tools-only_defconfig | 1 +
>  env/Kconfig                  | 7 +++++++
>  include/env_default.h        | 2 ++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
> index a853abf2b8fc..0ddd0518ef9a 100644
> --- a/configs/tools-only_defconfig
> +++ b/configs/tools-only_defconfig
> @@ -12,6 +12,7 @@ CONFIG_BOOTP_DNS2=y
>  CONFIG_OF_CONTROL=y
>  CONFIG_OF_HOSTFILE=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_EMPTY_DEFAULT_ENV=y
>  CONFIG_BOOTP_SEND_HOSTNAME=y
>  CONFIG_IP_DEFRAG=y
>  # CONFIG_ACPIGEN is not set
> diff --git a/env/Kconfig b/env/Kconfig
> index af4d9cbaa4d8..f7860f01cc16 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -614,6 +614,13 @@ config DEFAULT_ENV_FILE
>  	  containing key=value pairs, blank lines and lines beginning
>  	  with # are ignored.
>  
> +config EMPTY_DEFAULT_ENV
> +	bool "Create an empty default environment"
> +	help
> +	  Create an empty default environment. This is intended to be
> +	  used when building generic target tools and no sensible
> +	  default that can be included.
> +
>  config ENV_VARS_UBOOT_RUNTIME_CONFIG
>  	bool "Add run-time information to the environment"
>  	help
> diff --git a/include/env_default.h b/include/env_default.h
> index 8a0c3057f0aa..859188a8e7c9 100644
> --- a/include/env_default.h
> +++ b/include/env_default.h
> @@ -23,6 +23,7 @@ static char default_environment[] = {
>  const uchar default_environment[] = {
>  #endif
>  #ifndef CONFIG_USE_DEFAULT_ENV_FILE
> +#ifndef CONFIG_EMPTY_DEFAULT_ENV
>  #ifdef	CONFIG_ENV_CALLBACK_LIST_DEFAULT
>  	ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0"
>  #endif
> @@ -107,6 +108,7 @@ const uchar default_environment[] = {
>  #endif
>  #ifdef	CONFIG_EXTRA_ENV_SETTINGS
>  	CONFIG_EXTRA_ENV_SETTINGS
> +#endif
>  #endif
>  	"\0"
>  #else /* CONFIG_USE_DEFAULT_ENV_FILE */
> 


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[RFC PATCH] tools: env: Add an option to have an empty default environment

[Chris Packham]

On Tue, 18 Aug 2020, 11:02 PM Stefano Babic, <sbabic@denx.de> wrote:

> Hi Chris,
>
> On 13.08.20 03:37, Chris Packham wrote:
> > When building envtools via tools-only_defconfig the builtin defaults
> > are based on options in the defconfig. For example:
> >
> >   bootcmd=bootp; setenv bootargs root=/dev/nfs
> nfsroot=${serverip}:${rootpath}
> ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm
> >   bootdelay=2
> >   baudrate=115200
> >   stdin=serial,cros-ec-keyb,usbkbd
> >   stdout=serial,vidconsole
> >   stderr=serial,vidconsole
> >   ethaddr=00:00:11:22:33:44
> >   eth3addr=00:00:11:22:33:45
> >   eth5addr=00:00:11:22:33:46
> >   eth6addr=00:00:11:22:33:47
> >   ipaddr=1.2.3.4
> >   bootm_size=0x10000000
> >   kernel_addr_r=0x1000000
> >   fdt_addr_r=0xc00000
> >   ramdisk_addr_r=0x2000000
> >   scriptaddr=0x1000
> >   pxefile_addr_r=0x2000
> >
> > These may or may not be sensible for any particular target. Rather than
> > trying to have a set of defaults that work for every target add a config
> > option to make the default environment completely empty.
> >
> > Signed-off-by: Chris Packham <judge.packham@gmail.com>
> > ---
>
> Which is the added value compared to use CONFIG_USE_DEFAULT_ENV_FILE=y
> and adding your file with no variables ?
>

Long story short that was what I tried first but I was having trouble
making it work with tools-only_defconfig. But I think the point remains,
there's just no sane set of defaults that can be built-in to a generic
fw_setenv tool.


> Best regards,
> Stefano
>
> >
> >  configs/tools-only_defconfig | 1 +
> >  env/Kconfig                  | 7 +++++++
> >  include/env_default.h        | 2 ++
> >  3 files changed, 10 insertions(+)
> >
> > diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
> > index a853abf2b8fc..0ddd0518ef9a 100644
> > --- a/configs/tools-only_defconfig
> > +++ b/configs/tools-only_defconfig
> > @@ -12,6 +12,7 @@ CONFIG_BOOTP_DNS2=y
> >  CONFIG_OF_CONTROL=y
> >  CONFIG_OF_HOSTFILE=y
> >  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> > +CONFIG_EMPTY_DEFAULT_ENV=y
> >  CONFIG_BOOTP_SEND_HOSTNAME=y
> >  CONFIG_IP_DEFRAG=y
> >  # CONFIG_ACPIGEN is not set
> > diff --git a/env/Kconfig b/env/Kconfig
> > index af4d9cbaa4d8..f7860f01cc16 100644
> > --- a/env/Kconfig
> > +++ b/env/Kconfig
> > @@ -614,6 +614,13 @@ config DEFAULT_ENV_FILE
> >         containing key=value pairs, blank lines and lines beginning
> >         with # are ignored.
> >
> > +config EMPTY_DEFAULT_ENV
> > +     bool "Create an empty default environment"
> > +     help
> > +       Create an empty default environment. This is intended to be
> > +       used when building generic target tools and no sensible
> > +       default that can be included.
> > +
> >  config ENV_VARS_UBOOT_RUNTIME_CONFIG
> >       bool "Add run-time information to the environment"
> >       help
> > diff --git a/include/env_default.h b/include/env_default.h
> > index 8a0c3057f0aa..859188a8e7c9 100644
> > --- a/include/env_default.h
> > +++ b/include/env_default.h
> > @@ -23,6 +23,7 @@ static char default_environment[] = {
> >  const uchar default_environment[] = {
> >  #endif
> >  #ifndef CONFIG_USE_DEFAULT_ENV_FILE
> > +#ifndef CONFIG_EMPTY_DEFAULT_ENV
> >  #ifdef       CONFIG_ENV_CALLBACK_LIST_DEFAULT
> >       ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0"
> >  #endif
> > @@ -107,6 +108,7 @@ const uchar default_environment[] = {
> >  #endif
> >  #ifdef       CONFIG_EXTRA_ENV_SETTINGS
> >       CONFIG_EXTRA_ENV_SETTINGS
> > +#endif
> >  #endif
> >       "\0"
> >  #else /* CONFIG_USE_DEFAULT_ENV_FILE */
> >
>
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
> =====================================================================
>

[RFC PATCH] tools: env: Add an option to have an empty default environment

[Chris Packham]

Hi Wolfgang,

On Tue, 18 Aug 2020, 10:27 PM Wolfgang Denk, <wd@denx.de> wrote:

> Dear Chris Packham,
>
> In message <20200813013727.8186-1-judge.packham@gmail.com> you wrote:
> > When building envtools via tools-only_defconfig the builtin defaults
> > are based on options in the defconfig. For example:
> >
> >   bootcmd=bootp; setenv bootargs root=/dev/nfs
> nfsroot=${serverip}:${rootpath}
> ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm
> >   bootdelay=2
> >   baudrate=115200
> >   stdin=serial,cros-ec-keyb,usbkbd
> >   stdout=serial,vidconsole
> >   stderr=serial,vidconsole
> >   ethaddr=00:00:11:22:33:44
> >   eth3addr=00:00:11:22:33:45
> >   eth5addr=00:00:11:22:33:46
> >   eth6addr=00:00:11:22:33:47
>
> Having any specific MAC addresses set in the default environment has
> always been a strict No-Go as it is just begging for trouble.
>
> If this was a real example, that code should be fixed!!
>

Yes it's what you get if you build tools-only_defconfig as of 2020.07.
There's also various addresses that are bad for the boards I deal with.


> > These may or may not be sensible for any particular target. Rather than
> > trying to have a set of defaults that work for every target add a config
> > option to make the default environment completely empty.
>
> An empty default environment is even worse, as you are missing vital
> settings like baudrate, bootdelay etc.  There are good chances to
> brick your board with such settings.
>

I believe the baudrate has build time default that is suitable if nothing
is set. But yes there are things necessary for a board to boot an OS that
are undone by a blank environment. But the same also applies to the wrong
value. The current defaults render my boards expensive bricks.

What I think might be the best path forward is being able to supply
defaults via a file at run time. That way we can have a generic fw_setenv
but supply board specific defaults via a config file.


> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> The word "fit", as I understand it, means "appropriate to a purpose",
> and I would say the body of the Dean is supremely appropriate to  the
> purpose of sitting around all day and eating big heavy meals.
>                                  - Terry Pratchett, _Moving Pictures_
>

[RFC PATCH] tools: env: Add an option to have an empty default environment

[Wolfgang Denk]

Dear Chris,

In message <CAFOYHZBz3gYXH=a9bj1BEnM4ztVgxjDUYzQGCsJy47Exggb0TA@mail.gmail.com> you wrote:
>
> What I think might be the best path forward is being able to supply
> defaults via a file at run time. That way we can have a generic fw_setenv
> but supply board specific defaults via a config file.

This would indeed make sense (though I would not call this any
longer "default" then).

Patches welcome!

> Content-Type: text/html; charset="UTF-8"
> Content-Transfer-Encoding: quoted-printable

And please - stop sending HTML to the list!  Thanks.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Pull the wool over your own eyes!"                - J.R. "Bob" Dobbs