Change in osmo-bts[master]: allow to configure multiple oml remote-ip addresses

dexter gerrit-no-reply at
Tue Jun 29 14:18:36 UTC 2021

dexter has posted comments on this change. ( )

Change subject: allow to configure multiple oml remote-ip addresses

Patch Set 6:

File src/common/abis.c: 
PS5, Line 65: W
> from these names it's not entirely clear to me what the states represent. […]
> CONN means CONNECTED here? CONNECTING? Most probably needs to be changed to some of those, it's not  […]
> state is FAILED right? FAIL it's more like an event or action.
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. […]
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? 
PS5, Line 148: 	} else if ((struct llist_head *)priv->bsc_oml_host != (struct llist_head *)&bts-> {
> 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. 
PS5, Line 149: 		/* Get a BSC host from the list and move the list heade one position forward. */
> type: header
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 
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? 
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’
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. 
PS5, Line 206: 			       .name = "CONN",
PS5, Line 206: 	       .
> some strange space-based indenting here, at least it looks like that in gerrit
PS5, Line 207: 			       .in_event_mask = 0 | S(ABIS_LINK_EV_SIGN_LINK_DOWN)
> please drop the "0 |". […]
PS5, Line 207: 			       .in_event_mask = 0 | S(ABIS_LINK_EV_SIGN_LINK_DOWN)
> this 0| can be dropped
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. 
File src/common/main.c: 
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) 
File src/common/vty.c: 
PS5, Line 536: {
> iirc you are maintaining a pointer to the currently "connecting" remote ip. […]
This is now checked. Thanks.

To view, visit
To unsubscribe, or for help writing mail filters, visit

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I205f68a3a7f35fee4c38a7cfba2b014237df2727
Gerrit-Change-Number: 24513
Gerrit-PatchSet: 6
Gerrit-Owner: dexter <pmaier at>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at>
Gerrit-CC: fixeria <vyanitskiy at>
Gerrit-CC: pespin <pespin at>
Gerrit-Comment-Date: Tue, 29 Jun 2021 14:18:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at>
Comment-In-Reply-To: pespin <pespin at>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list