> On 8 Jun 2020, at 11:34, Martin Pieuchot <m...@openbsd.org> wrote:
>
> On 29/05/20(Fri) 13:22, Vitaliy Makkoveev wrote:
>> This time pppx_add_session() has mixed initialisation order. It starts
>> to initialise pipex(4) session, then initialises `ifnet', then links
>> pipex(4) session, then continue to initialize `ifnet'.
>> pppx_add_session() can sleep and pppx_if_start() can start to work with
>> unlinked pipex(4) session.
>>
>> Also pppx_if_destroy() can sleep and pppx_if_start() can access to this
>> `pxi' with unlinked session. pppx_if_start() has checking of
>> `IFF_RUNNING' flag but it's usesless.
>>
>> Diff below does pppx_add_session() reordering. At first we initilize
>> and attach `ifnet', at second we initialize and link pipex(4) session
>> and at last we set `IFF_RUNNING' flag on `ifnet.
>
> The fact that you have to call if_detach() if a field isn't validated
> makes me wonder if this refactoring is better than the existing logic.
Except "req->pr_timeout_sec !=0” checking all userland input checks
were copypasted prom pipex_add_session(). But pppx_add_session()
inserts pppx(4) related code between checks and session initialization.
So I decided to keep all pipex(4) related code together for further
deduplication. Also we want to initialise `ifnet’ before session
linking.
There is another way to rewrite pppx_add_session() and
pipex_add_session(). We can split pipex_add_session() to two
functions: pipex_init_session() and pipex_setup_session(). The first
will do checks and insert new session with PIPEX_STATE_INITIAL state.
The second will do the rest. So we can do checks before `ifnet’
dances in ppppx_add_session(). This time PIPEX_STATE_INITIAL declared
but not used, at least pipex_lookup_by_session_id() should be
refactored before.
Is this direction more acceptable?
>
>> Also we cleaning `IFF_RUNNING' flag before pppx(4) session destruction
>> in pppx_if_destroy().
>
> This seems unrelated.
>
>> Since we made `ifnet' and pipex(4) session initialization separately, we
>> are more close to remove duplicated code.
>
> Can't we do that directly? There are many blocks in this function:
>
> - Allocate a `pxi'
> - Attach an interface (`ifp')
> - Insert an address on the interface
> - Link the `pxi' with the interface
> - Setup the session
>
> Maybe user input validation should be done first.
What is you talk to do directly?
