Change in libosmocore[master]: gprs_ns2: dont use llist_for_each when freeing an element
gerrit-no-reply at lists.osmocom.org
Mon Aug 9 12:30:29 UTC 2021
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/25147 )
Change subject: gprs_ns2: dont use llist_for_each when freeing an element
Patch Set 1:
I have used this exact technique for the same reasons.
Freeing an item often has effects in FSMs / triggers cleanup actions which then tear down other instances as well.
(re dexter: FSM events get processed inline immediately, not on the next select().)
llist_for_each_entry_safe() is not actually safe: if the *next* entry gets deallocated from cleanup actions, then it falls on its face the same way llist_for_each_entry() does.
Getting the first entry of an llist is always safe, so it is the simplest and safest way to clear a list. The alternatives would be to implement an *actually* safe llist_for_each_entry, e.g. to compare against active iterators on each llist_del(); would be quite complex, and would not match the thin simple nature of the llist implementation.
For me personally, seeing this code is perfectly clear and I wouldn't ask for spreading comments explaining the same concept on every such clearing loop.
But would be nicer to have a single function like gprs_ns2_clear() instead of duplicating the loop all over. (Then there could also be a single comment in that function, explaining for the other reviewers...)
PS1, Line 1490:
looks like moving this free loop is fixing a separate issue?
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/25147
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Owner: lynxis lazus <lynxis at fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-CC: neels <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Mon, 09 Aug 2021 12:30:29 +0000
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the gerrit-log