Hi,
I've looked at the code -- it took me a few hours, wow -- so here are
a few initial comments.
First of all, great work! I'm glad most functions now take arguments,
instead of relying on a bunch of global variables. I'm also glad you
took this opportunity to refactor and fix $LANG handling,
i2p_is_enabled and various other ugly leftovers.
A general note: there are quite a few "2>/dev/null". I'm not sure what
it buys us, and I suspect it may make debugging more difficult than it
could be in the future.
The design doc should be updated, at least to point to the new shell
library file.
I'm surprised that we can now use 7z to unpack omni.ja (see fe54c8ca
for why we had to use unzip in the first place), but it's good news!
But then:
> + # Non-zero exit code due to non-standard ZIP archive.
> + # The following steps will fail soon if the extraction failed anyway.
> + 7z x -o"${tmp}" "${pack}" "${branding}" || true
Do we really still need "|| true"?
tails-shell-library/chroot-browser.sh now has code that was meant to
be run with "set -e". I believe it should be made clear at the top of
this file.
I'm not convinced by the renaming of run_browser_in_chroot to
run_chroot_browser, which feels less clear to me.
Commit c5eab441 would benefit from more explanations regarding why
patching torbutton@??? isn't needed anymore.
Maybe set_chroot_browser_name and set_chroot_torbutton_browser_name
should be merged? The latter's name make it look like a subset of
the former.
Regarding commit 9800961, it's unclear why this can be done for the
I2P browser, while the "the name is dictated completely by Torbutton"
assumption would not be verified for the Unsafe browser too.
> + rm -f ${chroot}/"${TBB_PROFILE}"/bookmarks.html
> + rm -f ${browser_profile}/bookmarks.html
> + rm -f ${browser_profile}/places.sqlite
How about doing it in a single "rm" call?
Also, I see little use in "-f" here, while it will possibly hide
issues (e.g. changed paths) later on.
Regarding commit 8de6bfd, best case this makes the chroot teardown
take 0.9 seconds less, right?
I see a few functions take a $browser_name argument, and then
hard-code something like
"/home/${chroot_user}/.${browser_name}/profile.default". Maybe we
could use a function that computes the path to the browser profile for
a given user & browser name? Not sure it's worth it.
Style nitpicking: I find it weird to on the one hand quote constant
strings with double-quotes consistently (e.g.
BROWSER_NAME="i2p-browser"), while on the other hand using constructs
like /my/string/"$variable" in many places. I would either *not* quote
constant strings ever, or write the latter "/my/string/$variable"
instead. Preferably the latter. What do you think?
> + when "the I2P Browser"
> + user = "i2pbrowesr"
XXX
I find the use of $startpage and $START_PAGE slightly confusing:
Firefox calls this the homepage, and in the context of Tails,
Startpage is our default search engine.
Regarding commit a1ad08b95 ("Optimistically try to tear down chroot if
killing failed"): why? :)
I'm not sure whether setup_browser_chroot() is the best place to set
up signal handlers, but not sure where it should live either.
I don't like commit 6a0e1448 ("Chain commands and report relevant
errors upon chain failure") very much. I suggest doing "|| error ..."
after each step, instead of building an ugly chain of commands with &&
(which is a bit confusing given we're running under "set -e").
I would rename localize_tails_doc_page to localized_tails_doc_page.
I'm not sure if it's really useful that language_code_from_locale
takes an argument ($LANG), instead of taking it the information it
needs from the environment. Especially since it's called
"from_locale".
Regarding commit 796883a9e ("Validate data (that we are gonna source)
from NetworkManager"), I suspect the regexp would be less ugly if it
used the extended syntax (-E).
Regarding commit 8051820d, this is an extra safety measure, useful
only if the checking added in previous commit fails to error out,
right? If so, I think adding a comment would be helpful.
I see an undocumented set_key() function is added to common.sh.
I think its name is much too generic for common.sh.
> + # We will use the detected language even if I2P supports it; it
> + # will default to English in that case.
Rather "even if I2P doesn't support it", right?
Not sure why the tails-i2p initscript lives in /usr/local/sbin, but
oh well.
That's all for today! :)
Cheers!
--
intrigeri
