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

[Andreas Pretzsch]

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 at 0 {
        [...]
        active_firmware_slot at 0 {
                [...]
        };
        [...]
};

this results in

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

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

# eval `barebox-state --dump-shell`
load successful
-sh: eval: state at 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 at cn-eng.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: barebox-state_fix_shell_escape.diff
Type: text/x-patch
Size: 950 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/barebox/attachments/20160404/ff4fd5a9/attachment.bin>