diff mbox series

[2/2] uboot-envtools: support alternate default config

Message ID 20201210124028.26979-2-bjorn@mork.no
State Superseded
Headers show
Series [1/2] uboot-envtools: add support for multiple config partitions | expand

Commit Message

Bjørn Mork Dec. 10, 2020, 12:40 p.m. UTC
Now that we can create an alternate configuration file, add support
for selecting it by using the alternate application names
`fw_printsys' or `fw_setsys'.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 package/boot/uboot-envtools/Makefile          |   2 +
 .../002-support-alternate-config.patch        |  91 ++++++++++
 .../patches/003-fix-usage-text.patch          | 167 ++++++++++++++++++
 3 files changed, 260 insertions(+)
 create mode 100644 package/boot/uboot-envtools/patches/002-support-alternate-config.patch
 create mode 100644 package/boot/uboot-envtools/patches/003-fix-usage-text.patch

Comments

Luis Araneda Dec. 11, 2020, 3:58 a.m. UTC | #1
Hi Bjørn,

On Thu, Dec 10, 2020 at 9:42 AM Bjørn Mork <bjorn@mork.no> wrote:
>
> Now that we can create an alternate configuration file, add support
> for selecting it by using the alternate application names
> `fw_printsys' or `fw_setsys'.
>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  package/boot/uboot-envtools/Makefile          |   2 +
>  .../002-support-alternate-config.patch        |  91 ++++++++++
>  .../patches/003-fix-usage-text.patch          | 167 ++++++++++++++++++
>  3 files changed, 260 insertions(+)
>  create mode 100644 package/boot/uboot-envtools/patches/002-support-alternate-config.patch
>  create mode 100644 package/boot/uboot-envtools/patches/003-fix-usage-text.patch

I think this could be archived without patching the upstream
fw_printenv tool, depending on what solution you find acceptable.

My motivation is to reduce the amount of patches that I think are not
going to be accepted upstream.
Please let me know if I'm missing some details that make the patches
required, so we can brainstorm alternative solutions.

One solution could be to add an alias using the "-c" option of
fw_printenv. This is its help text:
>  -c, --config         configuration file, default:/etc/fw_env.config

So, you could add the following alias:
# alias fw_printsys='fw_printenv -c /etc/fw_env2.config'

A second solution (assuming an alias is not acceptable), would be to
add a wrapper script, again using the "-c" option.
The wrapper script could contain:
> #!/bin/sh
> fw_printenv -c /etc/fw_env2.config "$@"

If you still want to modify the fw_printenv, I would suggest patching
it so it accepts a new argument with the function requested (print,
set) that it can be used to override the automatic detection based on
the cmd name.
I see you already have something similar to this on the second patch.
To me, this sounds like a modification that could be upstreamed.


Finally, another thing that I noticed from the first patch is that the
MTD partitions seems to be named u-boot-env2:
> find_mtd_index u-boot-env2

If that is the case, IMHO it would be more consistent to call the
command "fw_printenv2" instead of "fw_printsys".
Sure, it would not be consistent with Realtek's u-boot shell, but the
naming is more generic and could be used by other devices as well in
the future where the second env partition has something different than
system information.
In this case, variables like "cfgtype" could be renamed to "cfgnum" or
something else.

Regards,
Luis Araneda.
Bjørn Mork Dec. 11, 2020, 7:42 a.m. UTC | #2
Luis Araneda <luaraneda@gmail.com> writes:

> I think this could be archived without patching the upstream
> fw_printenv tool, depending on what solution you find acceptable.
>
> My motivation is to reduce the amount of patches that I think are not
> going to be accepted upstream.

Thanks a lot for your valuable feedback. I was not sure which way to
prefer.  This helps a lot.

> Please let me know if I'm missing some details that make the patches
> required, so we can brainstorm alternative solutions.
>
> One solution could be to add an alias using the "-c" option of
> fw_printenv. This is its help text:
>>  -c, --config         configuration file, default:/etc/fw_env.config
>
> So, you could add the following alias:
> # alias fw_printsys='fw_printenv -c /etc/fw_env2.config'

Did not think of that.  Too obvious, probably, Thanks.  But is there any
standard way of configuring system aliases from a package?  I believe
that's a prerequisite.  I'd like these commands to Just Work, preferably
also in a sysupgrade context although that could be solved by the
scripts having knowledge about the underlying config file.

(there are two system partitions.  If OpenWrt ever is to support the
second one, then sysupgrade will have to know about the way U-Boot
selects the next boot.  This is a variable in the "sys" environment)

> A second solution (assuming an alias is not acceptable), would be to
> add a wrapper script, again using the "-c" option.
> The wrapper script could contain:
>> #!/bin/sh
>> fw_printenv -c /etc/fw_env2.config "$@"

Yes, this was actually my first implementation.  It is pretty simple. I
just made it a bit too magic for myself by dealing with an aribtrary
number of configurations.  And then I wanted to "fix" the misleading
help text.  None of that is required..  So two very simple wrappers will
do.

Will send a v2 with this solution unless there are other contradicting
feedback.  And I will try to update the package revision this time...

> Finally, another thing that I noticed from the first patch is that the
> MTD partitions seems to be named u-boot-env2:
>> find_mtd_index u-boot-env2
>
> If that is the case, IMHO it would be more consistent to call the
> command "fw_printenv2" instead of "fw_printsys".
> Sure, it would not be consistent with Realtek's u-boot shell, but the
> naming is more generic and could be used by other devices as well in
> the future where the second env partition has something different than
> system information.
> In this case, variables like "cfgtype" could be renamed to "cfgnum" or
> something else.

Sorry, I really want to be consistent with U-Boot shell here.  We must
expect users with console access, and naming this enviroment differently
from the stock U-Boot will be unnecessarily confusing.

If the mismatch with the Linux/OpenWrt partition names is going to be a
problem, then I'd rather we change those to match stock.  Both realtek
devices I have (a Netgear GS108Tv3 and a ZyXEL GS1900-10HP) use these
partition names:

RTL838x# flshow
=============== FLASH Partition Layout ===============
Index  Name       Size       Address
------------------------------------------------------
 0     LOADER     0xe0000    0xb4000000-0xb40dffff
 1     BDINFO     0x10000    0xb40e0000-0xb40effff
 2     SYSINFO    0x10000    0xb40f0000-0xb40fffff
 3     JFFS2_CFG  0x100000   0xb4100000-0xb41fffff
 4     JFFS2_LOG  0x100000   0xb4200000-0xb42fffff
 5     RUNTIME1   0xe80000   0xb4300000-0xb517ffff
 6     RUNTIME2   0xe80000   0xb5180000-0xb5ffffff
======================================================

So "u-boot-env" is "BDINFO" and "u-boot-env2" is "SYSINFO".  The stock
U-Boot shell refers to the variable sets as "environment" for "BDINFO",
and "system information" for "SYSINFO":

 RTL838x# help printenv
 printenv - print environment variables
 
 Usage:
 printenv 
     - print values of all environment variables
 printenv name ...
    - print value of environment variable 'name'

 RTL838x# help printsys
 printsys - printsys - print system information variables

 Usage:
 printsys 
     - print values of all system information variables
 printenv name ...
     - print value of system information variable 'name'



Huh?  Did not notice that before, but that command name typo is
real. Yes, this is vendor patched and it sucks as expected ;-)

Anyway, the printsys/setsys/savesys along with SYSINFO is consistently
pointing to this as "system information" and not "environment 2".



Bjørn
diff mbox series

Patch

diff --git a/package/boot/uboot-envtools/Makefile b/package/boot/uboot-envtools/Makefile
index 601627011d56..e7ecfefec73a 100644
--- a/package/boot/uboot-envtools/Makefile
+++ b/package/boot/uboot-envtools/Makefile
@@ -68,6 +68,8 @@  define Package/uboot-envtools/install
 	$(INSTALL_DIR) $(1)/usr/sbin
 	$(INSTALL_BIN) $(PKG_BUILD_DIR)/tools/env/fw_printenv $(1)/usr/sbin
 	$(LN) fw_printenv $(1)/usr/sbin/fw_setenv
+	$(LN) fw_printenv $(1)/usr/sbin/fw_printsys
+	$(LN) fw_printenv $(1)/usr/sbin/fw_setsys
 	$(INSTALL_DIR) $(1)/lib
 	$(INSTALL_DATA) ./files/uboot-envtools.sh $(1)/lib
 	$(INSTALL_DIR) $(1)/etc/uci-defaults
diff --git a/package/boot/uboot-envtools/patches/002-support-alternate-config.patch b/package/boot/uboot-envtools/patches/002-support-alternate-config.patch
new file mode 100644
index 000000000000..aa341545a03c
--- /dev/null
+++ b/package/boot/uboot-envtools/patches/002-support-alternate-config.patch
@@ -0,0 +1,91 @@ 
+From e5255e1ca3af000adb5ff686ea5c5b5b60fb7d9d Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bmork@telenor.net>
+Date: Thu, 10 Dec 2020 12:32:21 +0100
+Subject: [PATCH 1/2] tools: env: add support for alternate config file
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Some devices use two distinct environment partitions for
+different variable sets.  Add basic support for choosing
+between two configuration files based on application name.
+
+Signed-off-by: Bjørn Mork <bmork@telenor.net>
+---
+ tools/env/fw_env_main.c    | 28 +++++++++++++++++++++++++---
+ tools/env/fw_env_private.h |  4 +++-
+ 2 files changed, 28 insertions(+), 4 deletions(-)
+
+diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c
+index 1d193bd437d8..d67c2ed407e3 100644
+--- a/tools/env/fw_env_main.c
++++ b/tools/env/fw_env_main.c
+@@ -42,6 +42,12 @@
+ #define CMD_SETENV	"fw_setenv"
+ static int do_printenv;
+ 
++#ifdef CONFIG_SYSFILE
++#define CMD_PRINTSYS	"fw_printsys"
++#define CMD_SETSYS	"fw_setsys"
++static int do_sys;
++#endif
++
+ static struct option long_options[] = {
+ 	{"config", required_argument, NULL, 'c'},
+ 	{"help", no_argument, NULL, 'h'},
+@@ -117,7 +123,7 @@ static void parse_common_args(int argc, char *argv[])
+ 	int c;
+ 
+ #ifdef CONFIG_FILE
+-	env_opts.config_file = CONFIG_FILE;
++	env_opts.config_file = do_sys ? CONFIG_SYSFILE : CONFIG_FILE;
+ #endif
+ 
+ 	while ((c = getopt_long(argc, argv, ":a:c:l:h:v", long_options, NULL)) !=
+@@ -219,10 +225,26 @@ int main(int argc, char *argv[])
+ 		do_printenv = 1;
+ 	} else if (strcmp(_cmdname, CMD_SETENV) == 0) {
+ 		do_printenv = 0;
++#ifdef CONFIG_SYSFILE
++	} else if (strcmp(_cmdname, CMD_PRINTSYS) == 0) {
++		do_printenv = 1;
++		do_sys = 1;
++	} else if (strcmp(_cmdname, CMD_SETSYS) == 0) {
++		do_printenv = 0;
++		do_sys = 1;
++#endif
+ 	} else {
+ 		fprintf(stderr,
+-			"Identity crisis - may be called as `%s' or as `%s' but not as `%s'\n",
+-			CMD_PRINTENV, CMD_SETENV, _cmdname);
++			"Identity crisis - may be called as `%s',"
++#ifdef CONFIG_SYSFILE
++			"`%s', `%s',"
++#endif
++			" or as `%s' but not as `%s'\n",
++			CMD_PRINTENV, CMD_SETENV,
++#ifdef CONFIG_SYSFILE
++			CMD_PRINTSYS, CMD_SETSYS,
++#endif
++			_cmdname);
+ 		exit(EXIT_FAILURE);
+ 	}
+ 
+diff --git a/tools/env/fw_env_private.h b/tools/env/fw_env_private.h
+index 86be16dabc62..35b9bfc70aee 100644
+--- a/tools/env/fw_env_private.h
++++ b/tools/env/fw_env_private.h
+@@ -23,7 +23,9 @@
+  */
+ #define CONFIG_FILE     "/etc/fw_env.config"
+ 
+-#ifndef CONFIG_FILE
++#ifdef CONFIG_FILE
++#define CONFIG_SYSFILE  "/etc/fw_sys.config"
++#else
+ #define HAVE_REDUND /* For systems with 2 env sectors */
+ #define DEVICE1_NAME      "/dev/mtd1"
+ #define DEVICE2_NAME      "/dev/mtd2"
+-- 
+2.29.2
+
diff --git a/package/boot/uboot-envtools/patches/003-fix-usage-text.patch b/package/boot/uboot-envtools/patches/003-fix-usage-text.patch
new file mode 100644
index 000000000000..220239ac2150
--- /dev/null
+++ b/package/boot/uboot-envtools/patches/003-fix-usage-text.patch
@@ -0,0 +1,167 @@ 
+From 2b3239c16749a175b189971a25025872774d690e Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bmork@telenor.net>
+Date: Thu, 10 Dec 2020 12:54:53 +0100
+Subject: [PATCH 2/2] tools: env: fix usage text for alternate cmd names
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Signed-off-by: Bjørn Mork <bmork@telenor.net>
+---
+ tools/env/fw_env_main.c | 65 ++++++++++++++++++++++-------------------
+ 1 file changed, 35 insertions(+), 30 deletions(-)
+
+diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c
+index d67c2ed407e3..034db42813e2 100644
+--- a/tools/env/fw_env_main.c
++++ b/tools/env/fw_env_main.c
+@@ -66,41 +66,46 @@ static int noheader;
+ /* getenv options */
+ static char *script_file;
+ 
+-void usage_printenv(void)
++void usage_common(const char *prog)
+ {
+-
++#ifdef CONFIG_FILE
++	const char *cfgfile = do_sys ? CONFIG_SYSFILE : CONFIG_FILE;
++#endif
+ 	fprintf(stderr,
+-		"Usage: fw_printenv [OPTIONS]... [VARIABLE]...\n"
+-		"Print variables from U-Boot environment\n"
++		"Usage: %s [OPTIONS]... [VARIABLE]...\n"
++		"%s variables from U-Boot environment\n"
+ 		"\n"
+ 		" -h, --help           print this help.\n"
+ 		" -v, --version        display version\n"
+ #ifdef CONFIG_FILE
+-		" -c, --config         configuration file, default:" CONFIG_FILE "\n"
++		" -c, --config         configuration file, default:%s\n"
++#endif
++		" -l, --lock           lock node, default:/var/lock\n",
++		prog, do_printenv ? "Print" : "Modify"
++#ifdef CONFIG_FILE
++		, cfgfile
+ #endif
++		);
++}
++
++void usage_printenv(const char *prog)
++{
++	usage_common(prog);
++	fprintf(stderr,
+ 		" -n, --noheader       do not repeat variable name in output\n"
+-		" -l, --lock           lock node, default:/var/lock\n"
+ 		"\n");
+ }
+ 
+-void usage_env_set(void)
++void usage_env_set(const char *prog)
+ {
++	usage_common(prog);
+ 	fprintf(stderr,
+-		"Usage: fw_setenv [OPTIONS]... [VARIABLE]...\n"
+-		"Modify variables in U-Boot environment\n"
+-		"\n"
+-		" -h, --help           print this help.\n"
+-		" -v, --version        display version\n"
+-#ifdef CONFIG_FILE
+-		" -c, --config         configuration file, default:" CONFIG_FILE "\n"
+-#endif
+-		" -l, --lock           lock node, default:/var/lock\n"
+ 		" -s, --script         batch mode to minimize writes\n"
+ 		"\n"
+ 		"Examples:\n"
+-		"  fw_setenv foo bar   set variable foo equal bar\n"
+-		"  fw_setenv foo       clear variable foo\n"
+-		"  fw_setenv --script file run batch script\n"
++		"  %s foo bar   set variable foo equal bar\n"
++		"  %s foo       clear variable foo\n"
++		"  %s --script file run batch script\n"
+ 		"\n"
+ 		"Script Syntax:\n"
+ 		"  key [space] value\n"
+@@ -115,10 +120,10 @@ void usage_env_set(void)
+ 		"  kernel_addr    400000\n"
+ 		"  foo            empty empty empty    empty empty empty\n"
+ 		"  bar\n"
+-		"\n");
++		"\n", prog, prog, prog);
+ }
+ 
+-static void parse_common_args(int argc, char *argv[])
++static void parse_common_args(char *prog, int argc, char *argv[])
+ {
+ 	int c;
+ 
+@@ -138,7 +143,7 @@ static void parse_common_args(int argc, char *argv[])
+ 			env_opts.lockname = optarg;
+ 			break;
+ 		case 'h':
+-			do_printenv ? usage_printenv() : usage_env_set();
++			do_printenv ? usage_printenv(prog) : usage_env_set(prog);
+ 			exit(EXIT_SUCCESS);
+ 			break;
+ 		case 'v':
+@@ -156,11 +161,11 @@ static void parse_common_args(int argc, char *argv[])
+ 	optind = 1;
+ }
+ 
+-int parse_printenv_args(int argc, char *argv[])
++int parse_printenv_args(char *prog, int argc, char *argv[])
+ {
+ 	int c;
+ 
+-	parse_common_args(argc, argv);
++	parse_common_args(prog, argc, argv);
+ 
+ 	while ((c = getopt_long(argc, argv, "a:c:ns:l:h:v", long_options, NULL))
+ 		!= EOF) {
+@@ -175,7 +180,7 @@ int parse_printenv_args(int argc, char *argv[])
+ 			/* ignore common options */
+ 			break;
+ 		default: /* '?' */
+-			usage_printenv();
++			usage_printenv(prog);
+ 			exit(EXIT_FAILURE);
+ 			break;
+ 		}
+@@ -183,11 +188,11 @@ int parse_printenv_args(int argc, char *argv[])
+ 	return 0;
+ }
+ 
+-int parse_setenv_args(int argc, char *argv[])
++int parse_setenv_args(char *prog, int argc, char *argv[])
+ {
+ 	int c;
+ 
+-	parse_common_args(argc, argv);
++	parse_common_args(prog, argc, argv);
+ 
+ 	while ((c = getopt_long(argc, argv, "a:c:ns:l:h:v", long_options, NULL))
+ 		!= EOF) {
+@@ -202,7 +207,7 @@ int parse_setenv_args(int argc, char *argv[])
+ 			/* ignore common options */
+ 			break;
+ 		default: /* '?' */
+-			usage_env_set();
++			usage_env_set(prog);
+ 			exit(EXIT_FAILURE);
+ 			break;
+ 		}
+@@ -249,10 +254,10 @@ int main(int argc, char *argv[])
+ 	}
+ 
+ 	if (do_printenv) {
+-		if (parse_printenv_args(argc, argv))
++		if (parse_printenv_args(_cmdname, argc, argv))
+ 			exit(EXIT_FAILURE);
+ 	} else {
+-		if (parse_setenv_args(argc, argv))
++		if (parse_setenv_args(_cmdname, argc, argv))
+ 			exit(EXIT_FAILURE);
+ 	}
+ 
+-- 
+2.29.2
+