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 4: Code-Review-1
(11 comments)
my earlier higher level reservations are resolved, but now I did find low level problems:
https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@300
PS4, Line 300: struct channel_mode_and_rate ch_mode_rate;
> put this in the user_plane scope?
this ^ should probably go inside the assignment{} scope instead, see other comments
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c
File src/osmo-bsc/assignment_fsm.c:
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@304
PS4, Line 304: static int check_requires_voice_stream(struct gsm_subscriber_connection *conn)
-1: code dup: please write one function that takes a chan_mode as input and returns a requires_voice_stream bool, and call this function twice; once for the primary chan_mode and once for the alternate chan_mode.
-1: actually, the requires_voice_stream must be identical between first and alternate preference, right?
-1: it doesn't make sense to fail the assignment if only one of the choices is not supported.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@377
PS4, Line 377: if (lchan_type_compat_with_mode(conn->lchan->type, &req->ch_mode_rate_pref)) {
-1: code dup: please implement this only once in a utility function, and call it twice: once for the primary choice, and if that failed then for the alt_pref.
Below, I expect it to look something like this:
if (check_for_existing_lchan(conn, &req->ch_mode_rate_pref)
|| check_for_existing_lchan(conn, &req->ch_mode_rate_alt)) {
/* re-using existing lchan succeeded. Discard FSM instance, it is not needed. */
osmo_fsm_inst_term(conn->assignment.fi);
return;
}
check_new_lchan(conn, &req->ch_mode_rate_pref);
if (!conn->lchan)
check_new_lchan(conn, &req->ch_mode_rate_alt);
if (!conn->lchan) {
assignment_fail(No lchan available...);
return;
}
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@443
PS4, Line 443: if (!conn->assignment.new_lchan && req->ch_mode_rate_alt_present) {
-1: code dup (same)
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@479
PS4, Line 479: fi = osmo_fsm_inst_alloc_child(&assignment_fsm, conn->fi, GSCON_EV_ASSIGNMENT_END);
(FYI, I considered whether it is better to allocate the FSM instance only *after* we checked whether the current lchan is sufficient, but IMHO it makes more sense to allocate it and use it as logging context. Also this makes sure the ASSIGNMENT_END signal gets dispatched to the conn FSM, which we apparently also forget to do in the re-use case at the moment. Also FYI, the channel re-use code path is completely broken for many other reasons, see https://gerrit.osmocom.org/c/osmo-bsc/+/12401 )
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@495
PS4, Line 495: if (check_for_existing_lchan(conn))
> ...maybe we can deallocate the assignment. […]
^ still valid, but don't worry about this one, see https://gerrit.osmocom.org/c/osmo-bsc/+/12401
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@501
PS4, Line 501: conn->ch_mode_rate = req->ch_mode_rate_pref;
-1: the ch_mode_rate use must not be ambiguous: does it apply to the current conn->lchan or to the new assignment.new_lchan?
The point is that if the assignment fails for whatever reason, you must be able to just discard the assignment information and keep on using the previous lchan without having modified any other state of the conn.
So, instead of modifying the conn->ch_mode_rate, you must stay inside the conn->assignment.* scope.
Only when the new lchan gets accepted and moved from conn->assignment.new_lchan to conn->lchan is when the conn->ch_mode_rate is allowed to be modified.
Maybe it makes more sense to place ch_mode_rate into the lchan struct instead?
Or if it is only used during assignment negotiation, put it in the conn->assignment{} scope.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@503
PS4, Line 503: /* In case the lchan allocation fails, we try with the alternat
("alternate". use line width.)
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@609
PS4, Line 609: static int select_codec(struct assignment_request *req, struct gsm0808_channel_type *ct,
"select_codecs" ?
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@632
PS4, Line 632: RATE_PREF_NONE);
(if full rate didn't work, then this must end up being half rate?
maybe pass RATE_PREF_HR explicitly, then?)
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@647
PS4, Line 647: RATE_PREF_NONE);
(RATE_PREF_FR?)
--
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: 4
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 16:55:52 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190124/a0d62129/attachment.htm>