barebox-state (dt-utils): dump-shell broken for state@0

[Andreas Pretzsch <apr@cn-eng.de>]

[-- Attachment #1: Type: text/plain, Size: 3508 bytes --]

On Mi, 2015-06-17 at 11:06 +0200, Sascha Hauer wrote:
> On Wed, Jun 17, 2015 at 08:42:50AM +0200, Jan Remmet wrote:
> > Hello,
> > I'm working with barebox states and wonder if there is already a tool to access
> > states in eeprom from linux?
> > The states from a dtb backend should be easy to read. For raw there must be
> > looked under /proc/devicetree or a state kernel driver.
> 
> Yes, there is such a tool here:
> 
> git://git.pengutronix.de/git/tools/dt-utils.git
> 
> The binary you'll need is barebox-state.

barebox-state provides a command "--dump-shell" to dump the state
suitable for shell sourcing.
Essentially, it prints out variable=value pairs.
So typical use would be a
	eval `barebox-state --dump-shell`
somewhere in a shell script (incl. simple ones like busybox ash).

Unfortunately, this breaks with indices in the state name, as @ is not
escaped.
Tested with dt-utils v2015.10.0, but according to code this is the case
from v2015.05.0 up until master.

As of commit 6d58ca4 "barebox-state: fix export of shell variables:",
the fixed prefix "STATE_" was replaced by the supplied state name.
Also in there, all '.' are replaced by '_' in the variable name. Not in
the state name itself.

The same would be necessary for (at least) '@', because it is invalid
also inside a shell variable name. As probably other chars, just I'm not
sure which might show up from the dts. So no premature patch from my
side.
Not only for the variable name, but also the state name itself.

For clarification of the setup and behaviour, see below.


Now, question would be how to fix this.
Also replacing '@' by '_' might break existing users (e.g. when parsing
from stdin or similar, instead of using shells eval command).
On the other hand, status quo breaks the "described" use of dump
_shell_ ...

Talking about this, it might be also some idea to resurrect the old
behaviour of always printing 'STATE_' as prefix instead of the state
name from dts. Not sure if it's the best idea, or how to call such an
extra option, didn't think it through by now.
Just saying, as I will go this way here (as a hotfix) for my setup...

Ideas, opinions ?

Best regards,
  Andreas




Having a dts definition (according to documentation) like

state: state@0 {
        [...]
        active_firmware_slot@0 {
                [...]
        };
        [...]
};

this results in

# barebox-state -vv
found state node /state@0:
state@0 {
        [...]
        active_firmware_slot@0 {
                [...]
        }
        [...]
}

# barebox-state --dump-shell
load successful
state@0_active_firmware_slot="slot1"
[...]

# eval `barebox-state --dump-shell`
load successful
-sh: eval: state@0_active_firmware_slot=slot1: not found


With attached (quick and dirty) patch, this would look like:

# barebox-state --dump-shell
load successful
state_0_active_firmware_slot="slot1"
[...]


Regarding these changes, some free() for the strdup might be nice,
albeit not really necessary, as main ends anyway soon.

Last IMHO, loglevel without passing verbose as parameter should be
reduced, resp. 'dev_info(&state->dev, "load successful\n");' should be
taken down in level. To stay silent if nothing went wrong, in good unix
tradition.


-- 

carpe noctem engineering
Ingenieurbuero fuer Hard- & Software-Entwicklung Andreas Pretzsch
Dipl.-Ing. (FH) Andreas Pretzsch        Tel. +49-(0)7307-936088-1
Lange Strasse 28a                       Fax: +49-(0)7307-936088-9
89250 Senden, Germany                   email: apr@cn-eng.de

[-- Attachment #2: barebox-state_fix_shell_escape.diff --]
[-- Type: text/x-patch, Size: 950 bytes --]

diff --git a/src/barebox-state.c b/src/barebox-state.c
index bc1010e..b7b6c03 100644
--- a/src/barebox-state.c
+++ b/src/barebox-state.c
@@ -2129,17 +2129,22 @@ int main(int argc, char *argv[])
 	if (do_dump_shell) {
 		state_for_each_var(state, v) {
 			struct variable_type *vtype;
-			char *name, *ptr;
+			char *statename, *name, *ptr;
 			int i;
 
-			/* replace "." by "_" to make it var name shell compatible */
+			/* replace "." by "_" to make the var name shell compatible */
 			name = strdup(v->name);
 			ptr = name;
 			while ((ptr = strchr(ptr, '.')))
 				*ptr++ = '_';
+			/* replace "@" by "_" to make the state name shell compatible */
+			statename = strdup(state->name);
+			ptr = statename;
+			while ((ptr = strchr(ptr, '@')))
+				*ptr++ = '_';
 
 			vtype = state_find_type(v->type);
-			printf("%s_%s=\"%s\"\n", state->name, name, vtype->__get(v));
+			printf("%s_%s=\"%s\"\n", statename, name, vtype->__get(v));
 		}
 	}
 

[-- Attachment #3: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox