Change in osmo-bsc[master]: assignment_fsm: use activate.info.s15_s0 for ASS. COMPL.

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Mon Feb 25 14:41:31 UTC 2019


Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13039 )

Change subject: assignment_fsm: use activate.info.s15_s0 for ASS. COMPL.
......................................................................


Patch Set 2: Code-Review-1

(1 comment)

https://gerrit.osmocom.org/#/c/13039/2//COMMIT_MSG
Commit Message:

https://gerrit.osmocom.org/#/c/13039/2//COMMIT_MSG@14
PS2, Line 14: lchan has changed. To prevent this, we must use
This patch seems to fix some confusion in the wrong direction. let me explain, probably repeating things you already know, bear with me plz:

recap:

- anything that is volatile data while the assignment is still ongoing should be in and only in conn->assignment.*.

- when the assignment is complete, the conn->assignment.* data becomes invalid (might be overwritten by a subsequent possibly bogus request) and the really valid data should be copied to conn->* 

- with lchan->activate.* it is the same idea. Anything that is volatile data while an lchan activation is in progress should be in and only in lchan->activate.*.

- When the lchan is activated, the lchan->activate.* data should be copied out to lchan->*. (Even though in this case the lchan->activate.* in practice remains valid as long as the lchan is active, I would still like this to follow the clean separation of currently used state from an incomplete activation state. The fact that it remains valid is only incidental, because an activation NACK would anyway completely break the lchan state in the current code.)

- lchan->activate.* should be entirely controlled by the lchan_fsm.c code.

- conn->assignment.* should be entirely controlled by the assignment_fsm.c code.

- assignment_fsm.c code should not "leak" volatile state into conn->*, and even more definitely not into lchan->*.

Here is what I think is currently wrong:

The assignment_fsm.c stores the chosen ch_mode_rate in lchan->ch_mode_rate, even though the assignment or activation are not complete yet, which violates above ideas and confuses transitory and accepted config.

So assignment_fsm_start() should not leak its state into lchan->*, but should keep a separate ch_mode_rate somewhere else, I'd say in conn->assignment.new_lchan_ch_mode_rate or so. (If it really needs to store it at all.)

This ch_mode_rate should get passed to lchan activation, where lchan_fsm.c then first stores it in lchan->activate.* as transitory state, and when the activation was ACKed copies it to lchan->ch_mode_rate.

That's how we ensure the lchan->* items reflect what is currently used, and transitory requests make their potential mess elsewhere.

Recently I merged a related patch: http://git.osmocom.org/osmo-bsc/commit/?id=4daa21076f823b2f1aa07581befa5ed77d95aead
Looking at that patch, I see now that it was also plastering across the above mistake. I shouldn't have added yet another lchan->s15_s0 field, but should have cleaned up use of lchan->ch_mode_rate, so that it becomes the definitive "this is what is used" data.

Is this making sense to you? If you like I can try to hack up a patch of what I mean, let me know whether you need that.



-- 
To view, visit https://gerrit.osmocom.org/13039
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: I9a7b3ce8646d641569eac24e202f44cdb5f67b3d
Gerrit-Change-Number: 13039
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Mon, 25 Feb 2019 14:41:31 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190225/d6d17ee6/attachment.htm>


More information about the gerrit-log mailing list