This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/24513 )
Change subject: allow to configure multiple oml remote-ip addresses
......................................................................
Patch Set 8: Code-Review-1
(4 comments)
https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c
File src/common/abis.c:
https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c@114
PS8, Line 114: if (g_bts->osmo_link) {
Are you sure you can ever get a ABIS_LINK_EV_SIGN_LINK_DOWN event where you have no oml_link nor rsl_link? I doubt so. Hence, oml_rsl_was_connected will always be true here, and the statechg to CONNECTING never happens.
https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c@150
PS8, Line 150: if (!llist_contains(&bts->bsc_oml_hosts, priv->bsc_oml_host))
I think I already explained in previous version that imho this is not the proper way of doing this. You should not keep and use pointer addresses to freed memory. You are only looking for a disaster here. What if a new entry has been allocated in the same memory chunk and added to the tail of the list? This condition would be true, so you'll go on and take next in list, which would be none, and be done. And in the process you could have jumped/discarded several addresses in the middle.
Rather than that, better send an event to the FSM from the VTY code before freeing the entry, and then drop the pointer (set it to NULL) in the FSM when receiving the event.
https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c@170
PS8, Line 170: osmo_get_macaddr(bts_dev_info.mac_addr, "eth0");
I see the eth0 comes from old version abis_open() below. This is probably hardcoded to sysmobts but it's not going to work fine on other systems? Anyway not related to this patch.
https://gerrit.osmocom.org/c/osmo-bts/+/24513/8/src/common/abis.c@223
PS8, Line 223: .out_state_mask = S(ABIS_LINK_ST_CONNECTING) | S(ABIS_LINK_ST_CONNECTED) | S(ABIS_LINK_ST_FAILED),
In general I find much more readable to have one state/event per line. It also makes new patches easier to review/apply/revert.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/24513
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I205f68a3a7f35fee4c38a7cfba2b014237df2727
Gerrit-Change-Number: 24513
Gerrit-PatchSet: 8
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 07 Jul 2021 09:00:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210707/04d04a66/attachment.htm>