dexter 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 6: (18 comments) https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c File src/common/abis.c: https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@65 PS5, Line 65: W > from these names it's not entirely clear to me what the states represent. […] Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@66 PS5, Line 66: ABIS_LINK_ST_CONN, > CONN means CONNECTED here? CONNECTING? Most probably needs to be changed to > some of those, it's not […] Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@67 PS5, Line 67: ABIS_LINK_ST_FAIL, > state is FAILED right? FAIL it's more like an event or action. Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@87 PS5, Line 87: static void abis_link_conn_action(struct osmo_fsm_inst *fi, uint32_t event, void *data) > we usually don't use "_action" suffix, only the name. […] Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@144 PS5, Line 144: /* In some implementations the user may specify a BSC host via commandline switch. If this is the case > (doesn't look clear to me to take cmdline related specialties here, but OK) This is related to osmo-bts-omldummy, this implementation has a commandline switch to specify the BSC host. Thats the reason why this parameter exists. I wonder if we could get rid of this if we just use the commandline parameter to create a list entry in the bsc_oml_hosts list. What do you think? https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@148 PS5, Line 148: } else if ((struct llist_head *)priv->bsc_oml_host != (struct llist_head *)&bts->bsc_oml_hosts.next) { > llist_first(&bts->bsc_oml_hosts)? Or do you actually mean the end of the > list? isn't that "previous" […] I detect want to detect the end of the list. Its indeed .prev. I have reorganized this a bit and made some functions for libosmocore to handle that better. https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@149 PS5, Line 149: /* Get a BSC host from the list and move the list heade one position forward. */ > type: header Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@153 PS5, Line 153: } else { > maybe moving this early termiantion above (if condition) would help making > the function a bit less c […] I think I can not do that. I need to check priv->dst_host and the end of the list first before I can know that the code should return/exit https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@163 PS5, Line 163: osmo_get_macaddr(bts_dev_info.mac_addr, "eth0"); > what is this? hardcoded eth0? this looks wrong This is probably only to get a unique identifier. It was already in the BTS code. Yes it looks wrong, especially since the name "eth0" is not used so much anymore. However this is a different topic, apparently this line never caused problems or nobody really cares about whats in bts_dev_info.mac_addr? https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@172 PS5, Line 172: if (line) { > drop {} The e1inp_line_get2 macro behaves strange when I leave the braces away. The original code also has the braces. I do not understand why it fails. The macro is defined as: #define e1inp_line_get2(line, USE) OSMO_ASSERT( osmo_use_count_get_put(&(line)->use_count, USE, 1) == 0 ); I also tried to leave the ; away, but it still does not compile. The error is: abis.c: In function ‘abis_link_connecting’: abis.c:174:2: error: ‘else’ without a previous ‘if’ else https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@176 PS5, Line 176: priv->line_ctr++; > not sure what this is about As I understand the line model in libosmo-abis each address (BSC) needs its own line. It is not possible to re-use a line with a different ip-address. It is also not possible to get rid of a line once it is created. https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@206 PS5, Line 206: .name = "CONN", > CONNECTING Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@206 PS5, Line 206: . > some strange space-based indenting here, at least it looks like that in gerrit Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@207 PS5, Line 207: .in_event_mask = 0 | S(ABIS_LINK_EV_SIGN_LINK_DOWN) > please drop the "0 |". […] Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@207 PS5, Line 207: .in_event_mask = 0 | S(ABIS_LINK_EV_SIGN_LINK_DOWN) > this 0| can be dropped Done https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/abis.c@216 PS5, Line 216: .in_event_mask = 0, > how does this work? A FSM state "DOWN" which does not permit any input > events, but which has multip […] It does not need any events. It has an onenter function that initiates the connection. If it succeeds it changes in the CONN(ECTED) state or it it may run into an unrecoverable problem, then it changes to the FAIL(ED) state. https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/main.c File src/common/main.c: https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/main.c@399 PS5, Line 399: if (llist_count(&g_bts->bsc_oml_hosts) == 0) { > what about the cmdline oml ip I saw in the fsm which was outisde the llist? This option only exists for osmo-bts-omldummy (see other comment) https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/vty.c File src/common/vty.c: https://gerrit.osmocom.org/c/osmo-bts/+/24513/5/src/common/vty.c@536 PS5, Line 536: { > iirc you are maintaining a pointer to the currently "connecting" remote ip. > […] This is now checked. Thanks. -- 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: 6 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Comment-Date: Tue, 29 Jun 2021 14:18:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <[email protected]> Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
