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

dexter gerrit-no-reply at lists.osmocom.org
Tue Jun 29 14:18:36 UTC 2021


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 <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
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 osmocom.org>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210629/7f2b049c/attachment.htm>


More information about the gerrit-log mailing list