Change in osmo-bsc[master]: bsc: lchan_rtp_fsm: Remove duplicate LCHAN_EV_RTP_RELEASED event

[Pau Espin Pedrol]

Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/11993 )

Change subject: bsc: lchan_rtp_fsm: Remove duplicate LCHAN_EV_RTP_RELEASED event
......................................................................


Patch Set 1:

(2 comments)

https://gerrit.osmocom.org/#/c/11993/1//COMMIT_MSG
Commit Message:

https://gerrit.osmocom.org/#/c/11993/1//COMMIT_MSG@23
PS1, Line 23: 20181128193707330 DAS <0012> fsm.c:381 assignment(conn4_0-0-6-TCH_F_PDCHasPDCH-0)[0x6120000024a0]{WAIT_LCHAN_ACTIVE}: Deallocated
> you *could* make this a bit more readable next time :)
My bad, I kept using same steps as in previous commits, but I was already using another BSC binary compiled which outputs tese long file paths. I'll fix it in next version of the patch.


https://gerrit.osmocom.org/#/c/11993/1/src/osmo-bsc/lchan_rtp_fsm.c
File src/osmo-bsc/lchan_rtp_fsm.c:

https://gerrit.osmocom.org/#/c/11993/1/src/osmo-bsc/lchan_rtp_fsm.c@a742
PS1, Line 742: 
> while it is true that this is normally a duplicate event, it does make sure that in case the RTP hit […]
I'm not sure yo get your point ntirely, let's see if I understand (after looking at the code again).

See _osmo_fsm_inst_term in fsm.c, in here we can see how the code path goes:

"""
	/* delete ourselves from the parent */
	parent = fi->proc.parent;
	if (parent) {
		LOGPFSMSRC(fi, file, line, "Removing from parent %s\n",
			   osmo_fsm_inst_name(parent));
		llist_del(&fi->proc.child);
	}

	/* call destructor / clean-up function */
	if (fi->fsm->cleanup)
		fi->fsm->cleanup(fi, cause);

	LOGPFSMSRC(fi, file, line, "Freeing instance\n");
	/* Fetch parent again in case it has changed. */
	parent = fi->proc.parent;
	osmo_fsm_inst_free(fi);

	/* indicate our termination to the parent */
	if (parent && cause != OSMO_FSM_TERM_PARENT)
		_osmo_fsm_inst_dispatch(parent, parent_term_event, data,
					file, line);
"""

It's seems quite clear to me that after cleanup it immediatelly follows the osmo_fsm_inst_dispatch(). BUT (I saw that now), only if cause != OSMO_FSM_TERM_PARENT, which means if lchan fsm calls "osmo_fsm_inst_term()", then it will call "osmo_fsm_inst_term_children(fi, OSMO_FSM_TERM_PARENT,...)" and then this one is needed.

So I think instead of dropping it, I should change the if condition to:
if (lchan->fi && cause == OSMO_FSM_TERM_PARENT)


agree?



-- 
To view, visit https://gerrit.osmocom.org/11993
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e95a21e5a5ec6c35b1ab20b7a642fd7eb81e556
Gerrit-Change-Number: 11993
Gerrit-PatchSet: 1
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 29 Nov 2018 15:32:11 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181129/7a28ece2/attachment.htm>