On 31/10/14 13:55, intrigeri wrote:
> 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.
Sure. I kept it in the `try_for()` blocks since it may flood stdout/err
with errors.
> The design doc should be updated, at least to point to the new shell
> library file.
Indeed.
> 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!
Perhaps that was an issue affecting Squeeze's 7z, but not Wheezy's?
> 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"?
Ah, it seems we don't any more. Also removed the comment.
> 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.
Added a comment.
> I'm not convinced by the renaming of run_browser_in_chroot to
> run_chroot_browser, which feels less clear to me.
Ok, fixed. For me the concept of the "chroot browser" is a browser whose
purpose is to be run in the chroot, but I agree that the old name is
clearer if one doesn't have that concept.
> Commit c5eab441 would benefit from more explanations regarding why
> patching torbutton@??? isn't needed anymore.
I do not understand. The set_chroot_torbutton_browser_name() function
does the patching, and it's called for the I2P browser so nothing is
lost. If you wonder why it's not called for the Unsafe browser, that's
because it doesn't have Torbutton installed.
To be clear: if Torbutton is installed, it overrides the browser
banding's name. That's why we rename the browser via Torbutton in the
I2P browser, since it has Torbutton installed, and why we rename it with
via the browser's branding in the Unsafe browser.
> 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.
I merged them but it doesn't become super pretty since the function now
needs more context (it needs to know which extensions are installed in
the profile) and depends on configure_chroot_browser().
> 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.
Please clarify what you mean here if you still think something is unclear.
>> + 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?
Actually the two latter files cannot exist, so I removed those lines.
> Also, I see little use in "-f" here, while it will possibly hide
> issues (e.g. changed paths) later on.
I agree, I removed that option.
> Regarding commit 8de6bfd, best case this makes the chroot teardown
> take 0.9 seconds less, right?
No, `try_for 10 ...` means "try for 10 seconds" (or more accurately,
"stop retrying this if 10 seconds has passed"; added a comment about
this).
> 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.
Well, I did it any way.
> 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?
Well, sure.
> 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.
Hehe, I didn't think about that. Changed it to $home_page/$HOME_PAGE.
> Regarding commit a1ad08b95 ("Optimistically try to tear down chroot if
> killing failed"): why? :)
Why not? With some luck, whatever process we failed to kill may die just
a bit later, and even it doesn't we may at least partially tear down the
chroot, which seems beneficial. If you think it doesn't make sense, I
can revert it.
> 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.
Me neither, so I'm not sure I can improve on it without further hints or
suggestions.
> 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 &&
I don't get what is ugly about it, and I actually like those constructs
a lot, so I wouldn't mind being enlightened why they're bad.
Any way, doing `|| error ...` after each step will be very verbose, and
will flood the translators with new strings without adding much benefit.
We could sidestep the latter by doing something like the following for
each of the calls in each chain:
configure_chroot_dns_servers "${CHROOT}" "${IP4_NAMESERVERS}" || \
FUN=configure_chroot_dns_servers() && \
error "`gettext \"Failed to configure browser (in \${FUN}).\"`"
Is this really any better? Do you have something nicer in mind? IMHO the
current situation with the chains is a pretty sweet tradeoff between
error reporting accuracy and script verboseness/complexity.
> (which is a bit confusing given we're running under "set -e").
I don't get why it's more confusing together with `set -e`, in fact `set
-e` is irrelevant in the &&-chain + ||-error-catching construct. What am
I missing?
In any case, after commit 5d16b59 all this is moot, except one instance
of chaining (for `copy_extra_tbb_prefs()`).
> I would rename localize_tails_doc_page to localized_tails_doc_page.
Thanks, that was a typo!
> 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.
I could imagine there being situations where you may want to do it for
another LANG than what you have set, perhaps at build time when we are
juggling with all supported languages.
> Especially since it's called "from_locale".
So you mean that for you "locale" always refers to the specific, set
locale (in the environment), not just *a* 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).
Sure, fixed.
> 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.
Hmm. True. What about `set_simple_config_key()`?
>> + # 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?
Right, thanks!
> Not sure why the tails-i2p initscript lives in /usr/local/sbin, but
> oh well.
There wasn't much thought behind that, I think. The old I2P start script
was also located there, probably because it wasn't started during init,
but via a GNOME menu entry. That was still the case when we "migrated"
to the tails-i2p script, and only a release after that did we make it
start via a NetworkManager hook. I don't really see it as an init script
any way, more like wrapper since we want to wrap extra functionality
around it.
Cheers!
