Alan wrote (23 Nov 2013 13:25:18 GMT) :
> Ticket: https://labs.riseup.net/code/issues/6436
Reviewed, merged into bugfix/safer-persistence so both are
merged together.
The documentation was not entirely updated. It still reads:
For example, to automatically install the `dia` software, a diagram
editor, and the `fontmatrix` software, a font manager, create a
`live-additional-software.conf` file with the following content:
Please fix this before merging into devel (no need to go through
a formal review'n'merge, just notify me so that I have another look
after you've merged).
Since we're discussing program design these days, I'm following up.
> def create_additional_packages_list():
> """Creates the additional packages list if it doesn't exist
> """
> if not has_additional_packages_list():
> syslog.syslog("Creating additional software configuration file")
> f = open(PACKAGES_LIST_FILE, 'w')
Given the purpose (and current use) of this function, that's only
called if has_additional_packages_list() is wrong, then if
has_additional_packages_list() is true when run a second time inside
the function, then it means that something is wrong, and silently
ignoring it doesn't seem wise to me. If you ask me, doing the same
check twice here does not add any robustness, it only shows a design
with blurry separation of concerns.
I would find it more elegant if:
* either consider it's part of the caller's contract to verify that
the creation action is needed, and then I would turn the "if not
..." check in the function body into an assertion (throws an
exception if the precondition is not satisfied, using your
preferred Python assertion/contract/something library or by hand)
* or rename the function into
"ensure_additional_packages_list_exists" and not bother checking
the file existence on the caller side => responsibilities are
very clear.
... and if you're worried about some other process creating the file
in the meantime, then doing the same check twice in a row doesn't help
much anyway: as you noticed yourself when we were discussing something
else the other day, doing so only mitigates the TOCTOU a tiny bit.
(Wow, so many lines of discussion for so few lines of code.)
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
