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.
configuration-window.ui:
- Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway
tails-additional-software:
Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferredUse 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 fieldsshow_system_log(): the log file is not readable by amnesia, so gedit is not able to show itapt_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.
config.py:
- 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
written (example:
https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).
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
org.boum.tails.additional-software.policy:
message> is not translatable
persistence.py:
- 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
Subtasks
Related issues
- Related to #16060
- Related to #16061
- Related to #16062 (closed)
- Related to #16034
- Related to #16110 (closed)
- Blocks #14598 (closed)
Original created by @alant on 15838 (Redmine)