<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/13039">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13039/2//COMMIT_MSG">Commit Message:</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/13039/2//COMMIT_MSG@14">Patch Set #2, Line 14:</a> <code style="font-family:monospace,monospace">lchan has changed. To prevent this, we must use</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This patch seems to fix some confusion in the wrong direction. let me explain, probably repeating things you already know, bear with me plz:</p><p style="white-space: pre-wrap; word-wrap: break-word;">recap:</p><ul><li>anything that is volatile data while the assignment is still ongoing should be in and only in conn->assignment.*.</li></ul><ul><li>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->*</li></ul><ul><li>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.*.</li></ul><ul><li>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.)</li></ul><ul><li>lchan->activate.* should be entirely controlled by the lchan_fsm.c code.</li></ul><ul><li>conn->assignment.* should be entirely controlled by the assignment_fsm.c code.</li></ul><ul><li>assignment_fsm.c code should not "leak" volatile state into conn->*, and even more definitely not into lchan->*.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">Here is what I think is currently wrong:</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.)</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">That's how we ensure the lchan->* items reflect what is currently used, and transitory requests make their potential mess elsewhere.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Recently I merged a related patch: http://git.osmocom.org/osmo-bsc/commit/?id=4daa21076f823b2f1aa07581befa5ed77d95aead<br>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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/13039">change 13039</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/13039"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I9a7b3ce8646d641569eac24e202f44cdb5f67b3d </div>
<div style="display:none"> Gerrit-Change-Number: 13039 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 25 Feb 2019 14:41:31 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>