Change in osmo-bsc-nat[master]: Add bsc_nat fsm

[neels]

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662 )

Change subject: Add bsc_nat fsm
......................................................................


Patch Set 4:

(8 comments)

https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/include/osmocom/bsc_nat/bsc_nat_fsm.h 
File include/osmocom/bsc_nat/bsc_nat_fsm.h:

https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/include/osmocom/bsc_nat/bsc_nat_fsm.h@21 
PS4, Line 21: enum bsc_nat_fsm_states {
(usually the state enum can live in the foo_fsm.c file, outside code only needs the events enum)


https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/include/osmocom/bsc_nat/bsc_nat_fsm.h@33 
PS4, Line 33: extern struct osmo_fsm bsc_nat_fsm;
(no need to declare the fsm in the .h, it can be static in the .c file. We usually do osmo_fsm_register() and osmo_fsm_inst_alloc() in the foo_fsm.c file)


https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/include/osmocom/bsc_nat/bsc_nat_fsm.h@35 
PS4, Line 35: void bsc_nat_start(struct bsc_nat *bsc_nat);
does this do the same as BSC_NAT_FSM_EV_START? Then maybe the events enum can also move to the .c file?

The idea is to list here exactly those items that calling code uses to interact with the FSM inst.


https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c 
File src/osmo-bsc-nat/bsc_nat_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@34 
PS4, Line 34: #define X(s) (1 << (s))
we usually call this S(). I don't know why, maybe "shift"? all i know is that so far I've only seen it called S(). Also we usually define this just above the states array. Again no reason really.


https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@142 
PS4, Line 142: 	osmo_fsm_inst_state_chg(fi, BSC_NAT_FSM_ST_STOPPED, 0, 0);
seems like ST_STOPPING is not needed: the onenter directly changes state to another state. Just put this code in the st_stopped_onenter() function and transition directly to ST_STOPPED?


https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@159 
PS4, Line 159: 			X(BSC_NAT_FSM_ST_STOPPING),
(let me plug here the scheme i invented and use in my FSM definitions, which allows adding and removing states and events from masks by editing / copypasting a single line only:

  .out_state_mask = 0
          | S(ST_FOO)
          | S(ST_BAR)
          ,

)


https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@184 
PS4, Line 184: int bsc_nat_fsm_timer_cb(struct osmo_fsm_inst *fi)
seems like no timer cb is needed.
Otherwise rather 'return 1;' to request FSM termination.


https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/main.c 
File src/osmo-bsc-nat/main.c:

https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/main.c@150 
PS4, Line 150: 		bsc_nat_stop(g_bsc_nat, true);
osmo_select_shutdown_request() changes the osmo select loop to only handle pending writes and no longer read incoming packets. bsc_nat_stop() does not do that. Are you sure this is what you want?

EDIT: I just realized that osmo_select_shutdown_request() is called in st_stopped_onenter(). Still it seems unnecessary to me to separate the select shutdown request from main(), there doesn't seem to be a reason to "hide" the shutdown request behind two state transitions in a distant .c file?

I can imagine rather use plain osmo_select_shutdown_request() as it was, and call bsc_nat_stop() after the main loop in main() exited?



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc-nat
Gerrit-Branch: master
Gerrit-Change-Id: I7d52fa649c397582b18a1a7dcc40bb407f3b2c97
Gerrit-Change-Number: 26662
Gerrit-PatchSet: 4
Gerrit-Owner: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: neels <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Tue, 28 Dec 2021 12:45:37 +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/20211228/551f5707/attachment.htm>