On Sat, 23 Nov 2013 23:40:55 +0100 intrigeri <[email protected]> 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 _______________________________________________ tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev
