lwip: fix lwip thread_create() wrapper #9588
Conversation
miri64
added
Type: bug
miri64
added
the
CI: ready for build
| void *arg = params->arg; | ||
| mutex_unlock(¶ms->sync); | ||
| thread(arg); | ||
| /* TODO: free stack? */ |
There was a problem hiding this comment.
@kaspar030 any idea how we might do that? It's allocated here
There was a problem hiding this comment.
If we were sure that the thread is not going to be forcefully cancelled, it could be done in the wrapper, before returning.
There was a problem hiding this comment.
Depending on the architecture, the stack might still be used to write the return value, so this sounds like a bad idea ;-).
There was a problem hiding this comment.
@miri64 You are right. I just realized how stupid my comment was!! How can one going to release the stack if the function is still executing!
What we need is either:
- A termination/cancellation function
- A thread creation mechanism that handles stack allocations automatically.
Multithread programming sucks.
There was a problem hiding this comment.
Well, at the moment non of the threads started by lwIP terminate, so this is a more theoretical issue in case lwIP ever decides to have something that actually finishes ;-).
| abort(); | ||
| } | ||
| mutex_lock(¶ms.sync); |
There was a problem hiding this comment.
This usage of mutexes - unlocking in a different thread than the one it is locked from- is kind of non-standard. I happens to work with RIOT mutexes, but I don't think it's a behaviour that should be relied upon or guaranteed.
The "correct" primitive for this usage is either a semaphore or a condition variable (I think we don't have those in RIOT).
@kaspar030 What do you think about this use of semaphores?
There was a problem hiding this comment.
@jcarrano since mutexes are the only reliable core synchronization mechanism RIOT has (at the moment) I rather go for them. If it works in RIOT, why not use it in RIOT ;-) (as it is in several other places, e.g. asymcute).
PS: there is a bug in sema that actually causes problems in lwip already ;-) #7035
There was a problem hiding this comment.
I opened a question-bug: #9594 .
Contribution description
This is something @jcarrano noticed when testing out #9573. Casting a function without return value to a function with return value is obviously wrong, so we need a proper wrapper. The function in question is used by lwIP to abstract the system layer, so it is vital to lwIP running properly (this is why I set the major label as I think this should get two ACKs).
Issues/PRs references
None