Change in osmo-bts[master]: Introduce bts_shutdown FSM

[neels]

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

Change subject: Introduce bts_shutdown FSM
......................................................................


Patch Set 3:

(6 comments)

https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/include/osmo-bts/bts_shutdown_fsm.h 
File include/osmo-bts/bts_shutdown_fsm.h:

https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/include/osmo-bts/bts_shutdown_fsm.h@40 
PS3, Line 40: struct gsm_bts;
weird to declare an opaque struct without any API in this header requiring it


https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/src/common/bts_shutdown_fsm.c 
File src/common/bts_shutdown_fsm.c:

https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/src/common/bts_shutdown_fsm.c@34 
PS3, Line 34: 	[BTS_SHUTDOWN_ST_NONE] = { },
(you can just omit states that need no T set, it will be zero initialized implicitly)


https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/src/common/bts_shutdown_fsm.c@40 
PS3, Line 40: ((struct gsm_bts *)fi->priv)->T_defs
(since there is only one global struct gsm_bts, the T_defs could also be just a global pointer)


https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/src/common/bts_shutdown_fsm.c@89 
PS3, Line 89: 		.name = "WaitRampDownComplete",
(my personal humble opinion is that I want to be able to directly grep the src by state names seen in the logs, here "WAIT_RAMP_DOWN_COMPL" would allow that)


https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/src/common/gsm_data.c 
File src/common/gsm_data.c:

https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/src/common/gsm_data.c@43 
PS3, Line 43: 	{ .T=-1, .default_val=1, .desc="Grace time for ramp down complete during shutdown (s)" },
It could be good to de-collision with other T used in various osmo programs, see http://osmocom.org/issues/4539 .
osmo-bsc and osmo-msc already both use -1 and -2.
We haven't really discussed a scheme on those timer numbers yet, but still maybe better not add more "collisions" for the time being?

rgrep 'T=-'


https://gerrit.osmocom.org/c/osmo-bts/+/18903/3/src/common/gsm_data.c@295 
PS3, Line 295: 					       LOGL_INFO, bts_name);
(possible without a char buffer:

  osmo_fsm_inst_alloc(..., id=NULL);
  osmo_fsm_inst_update_id_f(fi, "bts%d", bts->nr);

hold on, there is only one BTS in osmo-bts ... why even add the number?
)



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

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I8f48f17e61c3b9b86342eaf5b8a2b1ac9758bde5
Gerrit-Change-Number: 18903
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy 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: Mon, 22 Jun 2020 17:40:22 +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/20200622/7eaf231d/attachment.htm>