<p><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="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:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/include/osmocom/bsc_nat/bsc_nat_fsm.h@21">Patch Set #4, Line 21:</a> <code style="font-family:monospace,monospace">enum bsc_nat_fsm_states {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(usually the state enum can live in the foo_fsm.c file, outside code only needs the events enum)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/include/osmocom/bsc_nat/bsc_nat_fsm.h@33">Patch Set #4, Line 33:</a> <code style="font-family:monospace,monospace">extern struct osmo_fsm bsc_nat_fsm;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(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)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/include/osmocom/bsc_nat/bsc_nat_fsm.h@35">Patch Set #4, Line 35:</a> <code style="font-family:monospace,monospace">void bsc_nat_start(struct bsc_nat *bsc_nat);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">does this do the same as BSC_NAT_FSM_EV_START? Then maybe the events enum can also move to the .c file?</p><p style="white-space: pre-wrap; word-wrap: break-word;">The idea is to list here exactly those items that calling code uses to interact with the FSM inst.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="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:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@34">Patch Set #4, Line 34:</a> <code style="font-family:monospace,monospace">#define X(s) (1 << (s))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@142">Patch Set #4, Line 142:</a> <code style="font-family:monospace,monospace"> osmo_fsm_inst_state_chg(fi, BSC_NAT_FSM_ST_STOPPED, 0, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@159">Patch Set #4, Line 159:</a> <code style="font-family:monospace,monospace"> X(BSC_NAT_FSM_ST_STOPPING),</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(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:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> .out_state_mask = 0<br> | S(ST_FOO)<br> | S(ST_BAR)<br> ,</pre><p style="white-space: pre-wrap; word-wrap: break-word;">)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/bsc_nat_fsm.c@184">Patch Set #4, Line 184:</a> <code style="font-family:monospace,monospace">int bsc_nat_fsm_timer_cb(struct osmo_fsm_inst *fi)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">seems like no timer cb is needed.<br>Otherwise rather 'return 1;' to request FSM termination.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/main.c">File src/osmo-bsc-nat/main.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662/4/src/osmo-bsc-nat/main.c@150">Patch Set #4, Line 150:</a> <code style="font-family:monospace,monospace"> bsc_nat_stop(g_bsc_nat, true);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662">change 26662</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26662"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bsc-nat </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I7d52fa649c397582b18a1a7dcc40bb407f3b2c97 </div>
<div style="display:none"> Gerrit-Change-Number: 26662 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 28 Dec 2021 12:45:37 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>