Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Androidboot support #13
Conversation
|
the date mangling looks pretty wrong, the date in initrd should be centrally set by the shipped fixrtc script, if fixrtc runs to late for you, please move the mangling of any dates to a script running after fixrtc got executed (i.e. to local-bottom). also, only relying on the recovery partlabel doesnt feel like being enough, i might have a u-boot based image and have defined a partition called recovery in my gadget snap to hold some rollback script or whatnot. we should look for additional ways to identify an android boot beyond the partition label (run abootimg -i on the recovery or boot partitions for example). |
alfonsosanchezbeato
commented
May 26, 2017
•
Edited 1 time
-
alfonsosanchezbeato
May 26, 2017
-
May 26, 2017
|
@ogra1 I have been investigating a bit more about dates and how the fixrtc script works. For this concrete device, the main issue is that there is no battery for the RTC, so the date is always the epoch on boot/reboot. The command dumpe2fs always produces dates very near to the epoch when run in the fixrtc script (or when UC has started indeed), like: Last mount time: Thu Jan 1 00:34:39 1970 as the moment when "mountroot" is run the date is still wrong, and will never be right due to the lack of battery. In case there is no network, systemd finally sets the date to its build time: $ date But this is not good enough to validate assertions, as most of them have "since" date posterior to systemd build time. This makes snapd initialization fail on first boot (always talking here about the no network case). Therefore, I added this change to have a modern enough date so assertions could be validated. This would make cases like a factory case when maybe you start the system (no network) and use a USB drive to add a system-user assertion, work as expected. Using the date of the folder where the snaps are stored seemed like a good one to start with, although maybe there is a better place to look at. |
|
Re: running abooimg to make sure this is an android system seems a good idea to me, will add that. |
|
lets fix fixrtc then please and not add additional code in other places ... we had it working on the phones just fine (even on ones without rtc), so whatever makes it not work here should be fixable/fixed in the existing script that corrects the time. |
|
To access files so we can get its dates fixrtc would need to run after "mountroot", but that is not the case and it is run by "mount_premount" call. And looking at a comment in the code this might be needed as one of the things fixrtc is for is to avoid issues with fsck. So this sort of fix would need to run somewhere else. It is also using a specific file from UC, so it would make more sense to have this in core-build than in the generic initramfs (although maybe this could be done in a more general way). Anyway, if you do not see set_sane_date function general enough I can keep it specific to the board and remove from this PR. |
|
well, i think we should just enhance fixrtc and use a more generic approach than looking at a snap related file like:
one other thing that just struck me ... i dont think we need to limit the copy_exec for abootimg to armhf, there might be x86 based android style bootloader setups too. |
alfonsosanchezbeato
added some commits
May 25, 2017
|
Thanks for the comments, branch repushed. I have removed the function for setting the date as the fix will be in the generic initramfs, and addressed the rest of the comments. |
|
@mvo5 could you please take a look at this PR? |
alfonsosanchezbeato
added some commits
Jun 5, 2017
|
@ogra1 just made a couple of fixes for issues that I noticed today |
| @@ -0,0 +1,190 @@ | ||
| +# -*- shell-script -*- vim:ft=sh: |
mvo5
Jun 8, 2017
Contributor
Just out of curiosity - how is this script triggered? via kernel cmdline "BOOT=bootloader-script" ? Or some other mechanism?
alfonsosanchezbeato
Jun 9, 2017
Contributor
Correct, this gets run by adding boot=bootloader-script to the kernel command line.
zyga
Jun 9, 2017
Contributor
Can you add this to the script or to a readme file? This is going to be pretty opaque to others without documentation
| +# Entry point: mountroot(). | ||
| +#--------------------------------------------------------------------- | ||
| + | ||
| +pre_mountroot() |
mvo5
Jun 8, 2017
Contributor
This and the following get_partition_from_label looks like (almost) verbatim copies from the script in ubuntu-core-rootfs. I wonder if we can share code here?
| +} | ||
| + | ||
| +# setup $rootmnt based on os/kernel snaps | ||
| +do_root_mounting() |
mvo5
Jun 8, 2017
Contributor
Same for this one, I wonder if we can share code with ubuntu-core-rootfs here. Probably needs a small refactor because the ubuntu-c-r is doing slightly more.
| +snap_try_core=$snap_try_core | ||
| +snap_try_kernel=$snap_try_kernel | ||
| +snap_core=$snap_core | ||
| +snap_kernel=$snap_kernel" > "$1" |
mvo5
Jun 8, 2017
Contributor
Should this try something more atomic like:
... >"$1".tmp
mv "$1".tmp "$1"
?
alfonsosanchezbeato
Jun 9, 2017
Contributor
Hmm, not sure that makes much sense for such a small file tbh. Maybe if the file was a big one...
zyga
Jun 9, 2017
Contributor
I agree with mvo, please make it like this. We really want to be reliable here.
ogra1
Jun 9, 2017
Contributor
well, even though the risk is small with a small file it is even smaller when moving a tmpfile ...
| + run_scripts /scripts/local-premount | ||
| + log_end_msg | ||
| + | ||
| + # always ensure writable is in a good state |
mvo5
Jun 8, 2017
Contributor
This comment is slightly misleading here, it seems it got copied from ubuntu-core-rootfs which will do an fsck but it looks like this code is not that.
|
Branch repushed after some refactoring to share code between scripts. |
| @@ -0,0 +1,190 @@ | ||
| +# -*- shell-script -*- vim:ft=sh: |
mvo5
Jun 8, 2017
Contributor
Just out of curiosity - how is this script triggered? via kernel cmdline "BOOT=bootloader-script" ? Or some other mechanism?
alfonsosanchezbeato
Jun 9, 2017
Contributor
Correct, this gets run by adding boot=bootloader-script to the kernel command line.
zyga
Jun 9, 2017
Contributor
Can you add this to the script or to a readme file? This is going to be pretty opaque to others without documentation
| +snap_try_core=$snap_try_core | ||
| +snap_try_kernel=$snap_try_kernel | ||
| +snap_core=$snap_core | ||
| +snap_kernel=$snap_kernel" > "$1" |
mvo5
Jun 8, 2017
Contributor
Should this try something more atomic like:
... >"$1".tmp
mv "$1".tmp "$1"
?
alfonsosanchezbeato
Jun 9, 2017
Contributor
Hmm, not sure that makes much sense for such a small file tbh. Maybe if the file was a big one...
zyga
Jun 9, 2017
Contributor
I agree with mvo, please make it like this. We really want to be reliable here.
ogra1
Jun 9, 2017
Contributor
well, even though the risk is small with a small file it is even smaller when moving a tmpfile ...
| @@ -0,0 +1,190 @@ | ||
| +# -*- shell-script -*- vim:ft=sh: |
mvo5
Jun 8, 2017
Contributor
Just out of curiosity - how is this script triggered? via kernel cmdline "BOOT=bootloader-script" ? Or some other mechanism?
alfonsosanchezbeato
Jun 9, 2017
Contributor
Correct, this gets run by adding boot=bootloader-script to the kernel command line.
zyga
Jun 9, 2017
Contributor
Can you add this to the script or to a readme file? This is going to be pretty opaque to others without documentation
| +snap_try_core=$snap_try_core | ||
| +snap_try_kernel=$snap_try_kernel | ||
| +snap_core=$snap_core | ||
| +snap_kernel=$snap_kernel" > "$1" |
mvo5
Jun 8, 2017
Contributor
Should this try something more atomic like:
... >"$1".tmp
mv "$1".tmp "$1"
?
alfonsosanchezbeato
Jun 9, 2017
Contributor
Hmm, not sure that makes much sense for such a small file tbh. Maybe if the file was a big one...
zyga
Jun 9, 2017
Contributor
I agree with mvo, please make it like this. We really want to be reliable here.
ogra1
Jun 9, 2017
Contributor
well, even though the risk is small with a small file it is even smaller when moving a tmpfile ...
| + echo -e "snap_mode=\nsnap_core=$snap_core\nsnap_kernel=$snap_kernel\nsnap_try_core=\nsnap_try_kernel=" > \ | ||
| + "${rootmnt}/${androiddir}/androidboot.env" | ||
| + fi | ||
| + mount -o bind "${rootmnt}/writable/androidboot" "${rootmnt}/boot/androidboot" |
|
I was looking at this code and I think I need to make a request for tests. This is a very critical piece of infrastructure yet it has no tests at all. I think we could use an approach similar to the one here where shell code is tested with some nice and simple python unit tests. Let us know what you think and if you need a hand to get started on this. |
|
i think the various shellcheck tests we already run on it are good enough ... functional tests can only be made with actual boot tests on physical boards (which also use the respective code path) ... i agree we should have that but AFAIK there is still SD Mux hardware missing for this. |
|
@ogra1 the point of unit tests is not to check that the syntax is not wrong or that a device can recover from failed kernel update but to check that each functional block does what it says. It is in no way a replacement of the other two kinds of checks. My comment suggested to add them to have at least some guarantee that we are not breaking things later and that rarely-executed branches operate as expected. |
|
Thanks for the reviews, branch re-pushed after addressing comment. Re: testing, as discussed on irc that probably is a big amount of work and should be handled separately. |
|
Thanks, agreed on the testing, we can do a followup, lets see what aces @zyga has up his sleeve :) |
alfonsosanchezbeato commentedMay 25, 2017
No description provided.