On Sat, 23 Nov 2013 23:40:55 +0100
intrigeri <intrigeri@???> wrote:
> 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).
>
Hopefully fixed, please merge if you're OK.
>
> 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.)
>
Thanks for the comments.
I tried to implement the 1st solution in
feature/additional-software-dont-check-conf-twice. Please have a look
(I didn't build an ISO to test it, but inside a running Tails it
works). I don't fromally ask for merging this one (too lazy to build
and test an ISO only for that).
Cheers
