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.orgNeels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
......................................................................
Patch Set 5: Code-Review-1
(3 comments)
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c
File src/osmo-bsc/assignment_fsm.c:
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c@340
PS5, Line 340: return -EINVAL;
before, I said "it doesn't make sense to fail the assignment if only one of the choices is not supported", but now I'm revising that: it is unlikely to get a GSM48_CMODE that we can't classify, so this is fine.
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c@497
PS5, Line 497: if (check_for_existing_lchan(conn)) {
instead of spraying CR comments I pushed a suggestion instead. Can you please take a look whether that is acceptable to you, and actually test it for me? thanks!
My changes to this patch set are a separate commit on branch neels/12625
http://git.osmocom.org/osmo-bsc/commit/?h=neels/12625
(If they are ok then squash into this one)
The reasons:
- code dup
- missing counter
- missing send_assignment_complete() error handling
- the req struct is coming in during the assignment_start() and should then remain unchanged, i.e. moved req->chan_mode_rate to conn.assignment
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c
File src/osmo-bsc/handover_fsm.c:
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c@716
PS5, Line 716: sc.cfg = conn->assignment.req.ch_mode_rate.s15_s0;
Noticing this problem that exists before this patch:
This is reaching into the wrong place. The conn->assignment is only valid *during* an assignment. If you need the ch_mode_rate after assignment is done, it has to be copied out of conn->assignment.
The idea is:
- assignment_req data represents the request coming in. the request can be accepted or denied. If it is accepted, pass req to assignment_start(). From now on req is cast in stone and is only the input data.
- during assignment, keep all state in and only in conn->assignment.*, leave conn->lchan and other conn->* unchanged.
- when assignment was successful, copy all relevant data to conn->lchan or conn->foo, so that now conn->assignment is unused.
- so that if we start another assignment (which might fail) we do not affect the conn->lchan state.
- if assignment fails, leave conn->* unchanged, hence just continue as if no assignment happened at all.
I am submitting a patch that puts the s15_s0 bits into struct gsm_lchan after activation, instead:
https://gerrit.osmocom.org/#/c/osmo-bsc/+/12734
--
To view, visit https://gerrit.osmocom.org/12625
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: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Assignee: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Wed, 30 Jan 2019 16:02:07 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190130/d2405a58/attachment.htm>