Re: [Tails-dev] Please review and merge feature/create-addi…

Delete this message

Reply to this message
Autore: Alan
Data:  
To: tails-dev
Oggetto: Re: [Tails-dev] Please review and merge feature/create-additional-software-config
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