Hi,
Alan wrote (11 Mar 2013 21:15:00 GMT) :
> I have a working basic implementation in
> feature/remember_installed_packages. If you want to give a hand, please
> have a look and/or test and give feedback.
Here's a first quick "static" review.
> /etc/NetworkManager/dispatcher.d/70-apt-upgrade.sh
This filename is misleading: it suggests the script runs apt-get
upgrade, which it (rightly) does not. I believe
70-upgrade-xyz-packages.sh would be a better name.
One also would have to find something sensible to put in place of xyz.
"persistent" is no valid candidate, since not all packages stored in
persistence are upgraded by this script -- nor installed by the other,
by the way, so the naming issue should be addressed everywhere else in
this branch too.
> +pp_log() {
Please ensure the function names are unique in a nicer way than with
a fixed prefix... like, by using more descriptive names (e.g.
rename `pp_install' to `install_persistent_packages').
> + # Log arguments to the logfile defined in $PP_LOGFILE
Please document the function's purpose in a way that can't be confused
with documenting the next shell command, e.g. by moving the function
doc just before the function (same for most other functions, not just
this one.)
> + echo "$@" >> "$PP_LOGFILE"
Please use syslog instead. See 20-time.sh for an example.
> +pp_notify_user() {
Please extract this function from 20-time.sh into
usr/local/lib/tails-shell-library/$WHATEVER.sh, instead of duplicating
the code.
> + # Shows the paths of persistent packageslists
> [...]
> + # Shows the list of all the persistent packages
"Shows" is a bit vague. "Print on standard output", perhaps?
The aforementioned naming issue is obvious in "all the persistent
packages".
> + echo ${persistent_dir}${PP_PACKAGES_LIST_FILE}
Any good reason not to quote the argument to echo?
> + for package_list in $(pp_get_packages_lists); do
> + cat "${package_list}"
> + done
"cat $(pp_get_packages_lists)" would be enough.
> +pp_install_packages() {
> + # Install packages given as arguments
> + local package
> + for package in ${@}; do
I think running apt-get once for all remembered packages would be more
efficient and more robust: it gives APT the big picture of what we
want, instead of asking it to paint it blindly bit by bit.
> + for package in ${@}; do
Using ${} in this construct seems overkill to me, but I'd be happy to
learn if it brings advantages I don't know about.
> + apt-get --quiet --no-remove --yes install
> "${package}" 2>&1
I think you want -o 'DPkg::Options::=--force-confold' too.
> >> $PP_LOGFILE || \
Well, no, please use your logging function instead of gratuitously
breaking abstraction layers you've setup.
> + pp_install_packages "${packages}"
Why pass the packages list as a single argument, while the
pp_install_packages documentation reads "Install packages given as
arguments"?
^
Actually, I'm not sure pp_install_packages is worth existing at all as
a separate function, but well.
> + # The command which installs all persistent packages
> [...]
> + # The command which upgrades all persistent packages
Same naming problem, again.
> + echo "Will install the following packages: ${packages}"
Why stdout instead of logging?
> + pp_notify_user "`gettext 'Tails persistence'`" "`gettext 'Tails is starting to upgrade persistent software.'`"
I believe gettext was not initialized yet, so this has little chances
to work. I'll be happy to review a further, tested version.
Also, I'm not convinced about all the strings displayed to the user,
and I'd like sajolida to have a look at it and suggest improvements.
> + apt-get --quiet update 2>&1 >> $PP_LOGFILE && \
log function, here too.
> + pp_notify_user "`gettext 'Tails persistence'`" "`gettext
> 'Tails failed to upgrade persistent software.'`"
Being given some clues about the actual issue (either displaying
error messages, or pointing to the log file) would be helpful.
In pp_upgrade, I'm not sure about what kind of user experience the
sequence of desktop notifications and their timeouts create.
Generally, I think I like it more when the first ("starting")
notification stays displayed until it's replaced by the
"finished" one.
> + # The command which shows help
This is not exactly a command. s/shows/displays.
> + echo "Usage: $0 [command]"
I don't think command is really optional here.
Also, it's customary to send usage output to stderr.
Also, it would be great to have some meaningful exit code for the
whole script.
> +++ b/config/chroot_local-patches/install-persistent-packages.diff
In case you're not aware, this will have to go into tails-greeter at
merge time. So perhaps just apply this patch to t-g right now,
wrapped with a [ -x /usr/local/sbin/tails-persistent-packages ]
test, so that t-g is ready for when we have this feature in the main
Tails dev branches? (The naming issue should be addressed first, to
minimize overhead, of course :)
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
