Merge lp://staging/~manjo/flash-kernel/flash-kernel-ubootvars into lp://staging/flash-kernel

Proposed by Manoj Iyer
Status: Needs review
Proposed branch: lp://staging/~manjo/flash-kernel/flash-kernel-ubootvars
Merge into: lp://staging/flash-kernel
Diff against target: 84 lines (+38/-1)
5 files modified
debian/changelog (+6/-0)
debian/dirs (+1/-0)
debian/flash-kernel.install (+1/-0)
functions (+29/-1)
ubootenv.d/default.conf (+1/-0)
To merge this branch: bzr merge lp://staging/~manjo/flash-kernel/flash-kernel-ubootvars
Reviewer Review Type Date Requested Status
dann frazier (community) Needs Fixing
Ubuntu Installer Team Pending
Review via email: mp+216787@code.staging.launchpad.net

Description of the change

Added support for user defined uboot environment variables.
/usr/share/flash-kernel/ubootenv.d/default.conf - to add uboot environment variables that apply to all boards
/etc/flash-kernel/ubootenv.d/ - where package/user can drop custom uboot env files to override defaults.

To post a comment you must log in.
Revision history for this message
Manoj Iyer (manjo) wrote :

== adding new uboot env var ==
ubuntu@c1n1:~$ cat /usr/share/flash-kernel/ubootenv.d/default.conf
# Enter default uboot environment variables here

ubuntu@c1n1:~$ cat /etc/flash-kernel/ubootenv.d/ti.conf

setenv mem2_reserve 5120M

ubuntu@c1n1:~$ sudo flash-kernel

ubuntu@c1n1:~$ dd if=/boot/boot.scr of=boot.4.script bs=72 skip=1
13+1 records in
13+1 records out
947 bytes (947 B) copied, 0.00103475 s, 915 kB/s

ubuntu@c1n1:~$ cat boot.4.script
setenv mem2_reserve 5120M
setenv interface scsi
echo Starting Ubuntu...
(more lines follow)

== overriding existing vars ==
ubuntu@c1n1:~$ cat /usr/share/flash-kernel/ubootenv.d/default.conf
# Enter default uboot environment variables here
setenv mem2_reserve 1204M

ubuntu@c1n1:~$ cat /etc/flash-kernel/ubootenv.d/ti.conf

setenv mem2_reserve 5120M

ubuntu@c1n1:~$ sudo flash-kernel

ubuntu@c1n1:~$ dd if=/boot/boot.scr of=boot.4.script bs=72 skip=1
13+1 records in
13+1 records out
947 bytes (947 B) copied, 0.00103502 s, 915 kB/s

ubuntu@c1n1:~$ cat boot.4.script
setenv mem2_reserve 5120M
setenv interface scsi
echo Starting Ubuntu...
(more lines follow)

Revision history for this message
dann frazier (dannf) wrote :

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

Revision history for this message
dann frazier (dannf) wrote :

I needed this functionality for a feature I'm trying to add (retaining installtime cmdline), so I went ahead and made the changes I noted and pushed it here:

 https://code.launchpad.net/~dannf/ubuntu/trusty/flash-kernel/ubootenv.d

Feel free to start with that patch, or not. A couple of other comments I noticed while doing this:

 - I'm not sure why the /etc and /usr ubootenv dirs are being added to the package in different ways. You should be able to put both in debian/dirs.
 - append_ubootvars is actually a misnomer since we're not appending the vars, we're prepending them.

review: Needs Fixing

Unmerged revisions

750. By Manoj Iyer

updated changelog

749. By Manoj Iyer

Added support for user defined uboot environment variables

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches