Code review comment for lp://staging/~manjo/flash-kernel/flash-kernel-ubootvars

Thanks for implementing this - some comments.

I don't see any good way a package can interact with this. The only files that are used in this implementation are /usr/share/flash-kernel/ubootenv.d/default.conf and files dropped under /etc/flash-kernel/ubootenv.d/. It seems like we'd want a package to be able to drop a file under /usr (e.g. /usr/share/flash-kernel/ubootenv.d/ti.conf). Policy dictates that users and other packages cannot modify this file. We can then allow a user to override this file by dropping in an override file in /etc/flash-kernel/ubootenv.d.

I'd like to see us avoid trying to parse these files. In this proposal, it looks like flash-kernel is trying to understand what variables a user is overriding by parsing the contents. Instead, we could just take these snippets as "black boxes" that we just include entirely. This would give us more flexibility - we could add *any* u-boot commands, not just variable setting.

The "find" seems a little overkill - I don't think we need to support subdirs of ubootenv.d, just files directly inside of it. Also, the find command will return files we probably don't want (e.g. ti.conf~, #ti.conf#, etc). I think we should adopt a policy for acceptable filenames - ideally the same thing as initramfs-tools:

From initramfs-tools(8):
       Valid boot and hook scripts names consist solely of alphabetics, numerics, dashes and
       underscores. Other scripts are discarded.

We might also want to adopt a predictable ordering - perhaps the same ordering used by run-parts (lexical sort order w/ LOCALE=C according to run-parts(8)).

With these changes, the loop become something simpler like:

ENVSTUBDIRS="/etc/flash-kernel/ubootenv.d /usr/share/flash-kernel/ubootenv.d"
# This find command should probably only match a run-parts
# like regex...
ENVSTUBS="$(find $ENVSTUBDIRS -type f -printf '%f\n')"
for file in $ENVSTUBS; do
  for dir in $ENVSTUBDIRS; do
  if [ -f $dir/$file ]; do
    cat $dir/$file
    break
  fi
done > $tmp_conf_file