Change in libosmocore[master]: gprs_ns2: dont use llist_for_each when freeing an element

neels 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:

(1 comment)

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...)

https://gerrit.osmocom.org/c/libosmocore/+/25147/1/src/gb/gprs_ns2.c 
File src/gb/gprs_ns2.c:

https://gerrit.osmocom.org/c/libosmocore/+/25147/1/src/gb/gprs_ns2.c@1490 
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-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I902557fb6e56e6588728a46e43a9cbe3215d5c68
Gerrit-Change-Number: 25147
Gerrit-PatchSet: 1
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
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210809/5bc87d3c/attachment.htm>


More information about the gerrit-log mailing list