Fix non-blocking issues identified during ASP code review
My first batch of remarks:
I’d suggest you set `PERSISTENCE_SETUP_USERNAME = “tails-persistence-setup”` in tailslib/init.py and use it in the two occurrances where this string is currently used.
- Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway
Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred Use logging.warning instead of deprecated logging.warn _notify_failure(): * incomplete sentence in docstring: “The user has the option to edit the configuration of to view the system log” * second “details =” line uses .format() but the string doesn’t contain any replacement fields show_system_log(): the log file is not readable by amnesia, so gedit is not able to show it
- -apt_hook_pre(): you shouldn’t use `assert` to check for conditions that could happen, because it is only evaluated if debug is True (see https://docs.python.org/3/reference/simple_stmts.html)-
main(): What do we need the syslog handler for? Wouldn’t it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.
- there is a race condition between multiple calls of
add_additional_packages and/or remove_additional_packages: if
the config file is written between when the packages are read and
when the packages are written, this change will be overwritten by
the subsequent write. You could fix this by acquiring a file lock
before reading the file, for example with fcntl.flock(f,
fcntl.LOCK_EX), and only releasing the lock once the file was
This way you could also get rid of the python3-atomicwrites package
it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager
- both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?
- as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True
message> is not translatable
- get_persistence_path(): only ever called with search_new_persistence=True, return_nonexistent=False
- has_unlocked_persistence(): only called with search_new_persistence=True
Feature Branch: bugfix/15838-asp-fix-non-blocking-issues