Attention is currently required from: pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816 )
Change subject: add ps_rab_ass FSM to map GTP via UPF
......................................................................
Patch Set 1:
(11 comments)
File include/osmocom/hnbgw/context_map.h:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/5f3a5fe8_9670b6ad
PS1, Line 56: * and each Response gets one ps_rab_ass FSM. Each such message can contain any number and any combination of
> you mean each req+response pair has a ps_rab_ass? or one for each?
one each. tried to think of an ascii art way to draw the relations because it is strugglesome to explain in words ...
first i tried to have one FSM for req + resp + PFCP for all RABs, then gradually noticed that each RAB needs a separate PFCP FSM, and then that i also need to have separate FSMs for RANAP req and resp.
File include/osmocom/hnbgw/ps_rab_fsm.h:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/49c62d11_8063c541
PS1, Line 24: struct half_gtp_map {
> gtp_map_leg? to use some term more "usual".
gtp_tunnel would be accurate, except that the FAR (Forwarding Action Rule) is closely related to the *other* GTP tunnel. I'll expand the comment.
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/8ec7ec53_37a06b83
PS1, Line 36: /* Whether the RANAP message this RAB's remote address was obtained from encoded the address in x213_nsap */
> "from encoded the address" looks wrongly written?
it is accurate but i'll rephrase to less ambiguous grammar
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/80c397c7_60c89080
PS1, Line 54: /* Backpointer to the ps_rab_ass_fsm for the RAB Assignment Request from Core that created this RAB. */
> (I wonder why do we need 2 different FSMs here, looks like the same followup procedure to me, req + […]
commenting in code...
File src/osmo-hnbgw/hnbgw.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/2bc0f340_b5d5eb4d
PS1, Line 230: LOGP(DHNBAP, LOGL_INFO, "deallocating UE context: id 0x%x, imsi %s, tmsi 0x%x\n",
> this can probably go on a different commit.
missed this one, i think i needed it for debugging, could just drop it
File src/osmo-hnbgw/hnbgw_cn.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/0b5c7053_b1792c48
PS1, Line 373: if (!map->is_ps) {
> not super important, but probably makes it easier to read if you do "if (map->is_ps)" now, and move […]
we (i?) usually list CS above PS ... so the is_ps maybe should have been named is_cs instead; i agree that it is slightly annoying, but i've made peace with that over the years
(this line is unchanged from before this patch despite gerrit rendering on green background, I'm only adding an 'else')
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/6c9af076_7cd2ecf3
PS1, Line 396: rc = ranap_ran_rx_co_decode(map, message, msgb_l2(oph->msg), msgb_l2len(oph->msg));
> You are actually duplicating these 2 lines now. They can be moved outside the if/else condition. […]
i think i even considered that when writing the patch, but left it like this to show that the CS code is completely unchanged... but true.
i'll remove the code dup in a separate patch
File src/osmo-hnbgw/hnbgw_pfcp.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/6254cfaf_7516e07d
PS1, Line 77: if (!hnb_gw_is_gtp_mapping_enabled(hnb_gw)) {
> I'd move this to the main function, to avoid calling hnbgw_pfcp_init(). […]
that's a matter of taste ... personally i like it very much when things are completely contained in a single file, with the parts appearing in main() as minimal as possible.
but i see the LOGP above seems at the wrong place
File src/osmo-hnbgw/hnbgw_rua.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/3b1f3a66_383eaeb2
PS1, Line 284: if (!map->is_ps) {
> same, probably easier to read with "if (map->is_ps)"
idk i like CS above PS better, also before this patch there was "!is_ps" above
File src/osmo-hnbgw/hnbgw_vty.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/975c18f5_a6158113
PS1, Line 474: if (g_hnb_gw->config.pfcp.local_addr)
> I think we usually print first local addresses, then remote addresses.
same order here as where the cmds are added to vty, but ok whatever
File src/osmo-hnbgw/ps_rab_ass_fsm.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/4eec37a5_8a12da06
PS1, Line 490: /* See whether all RABs are done establishing, and replace RTP info in the RAB Assignment Response message, so that we
> s/RTP/GTP/
lol thx
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Ic9bc30f322c4c6c6e82462d1da50cb15b336c63a
Gerrit-Change-Number: 28816
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 28 Jul 2022 10:03:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-cbc/+/28826
to look at the new patch set (#2).
Change subject: Split event list for smscb_message_fsm and smscb_peer_fsm
......................................................................
Split event list for smscb_message_fsm and smscb_peer_fsm
It really doesn't make sense to share the event list betwen those for
several reasons.
First, because ech of those FSM relate to different things:
* smscb_message_fsm: Handling/scheduling of 1 smscb towards all peers
* smscb_peer_fsm: Handling/scheduling of 1 smscb towards 1 peer.
The former has higher level interface used by the REST
API, plus some mid level interface used by smscb_peer_fsm.
The later has a mid level interface used by smscb_message_fsm to
interact, and a lower level interface used by the SBcAP/CBSP links to
talk to.
Furthermore, this makes it a lot easier understanding which events are
sent from one to another FSM.
Change-Id: I909474d1ff4ec7ed20aff0981da47074397df6cb
---
M include/osmocom/cbc/Makefile.am
M include/osmocom/cbc/smscb_message_fsm.h
A include/osmocom/cbc/smscb_peer_fsm.h
M src/cbc_message.c
M src/cbsp_link_fsm.c
M src/sbcap_link_fsm.c
M src/smscb_message_fsm.c
M src/smscb_peer_fsm.c
8 files changed, 197 insertions(+), 179 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-cbc refs/changes/26/28826/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-cbc/+/28826
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-cbc
Gerrit-Branch: master
Gerrit-Change-Id: I909474d1ff4ec7ed20aff0981da47074397df6cb
Gerrit-Change-Number: 28826
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin.
pespin has removed a vote from this change. ( https://gerrit.osmocom.org/c/osmo-cbc/+/28823 )
Change subject: sbcap: Store reported failed TAIs from WriteReplaceResponse in cbc_message_peer
......................................................................
Removed Verified-1 by Jenkins Builder (1000002)
--
To view, visit https://gerrit.osmocom.org/c/osmo-cbc/+/28823
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-cbc
Gerrit-Branch: master
Gerrit-Change-Id: I37b08aa374c1097d2871bf69a7bb7893f32bccd3
Gerrit-Change-Number: 28823
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: deleteVote
Attention is currently required from: fixeria.
Hello Jenkins Builder, fixeria,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-cbc/+/28819
to look at the new patch set (#2).
Change subject: sbcap: Log info about messages received and trasmitted
......................................................................
sbcap: Log info about messages received and trasmitted
Change-Id: Ifd1eca79bf4fac63de8066cd6a18004138d51d04
---
M include/osmocom/sbcap/sbcap_common.h
M src/sbcap/sbcap_common.c
M src/sbcap_link.c
M src/sbcap_link_fsm.c
4 files changed, 65 insertions(+), 20 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-cbc refs/changes/19/28819/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-cbc/+/28819
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-cbc
Gerrit-Branch: master
Gerrit-Change-Id: Ifd1eca79bf4fac63de8066cd6a18004138d51d04
Gerrit-Change-Number: 28819
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-cbc/+/28819 )
Change subject: sbcap: Log info about messages received and trasmitted
......................................................................
Patch Set 1:
(2 comments)
File src/sbcap_link.c:
https://gerrit.osmocom.org/c/osmo-cbc/+/28819/comment/de5bc368_4127b1c9
PS1, Line 397: Transmitting
> Given that you're changing "Received" to "Rx", I suggest to use "Tx" here for consistency.
Ack
https://gerrit.osmocom.org/c/osmo-cbc/+/28819/comment/88b42868_4a92312f
PS1, Line 403: LOGPSBCAPC(link, LOGL_DEBUG, "Encoded message %s: %s\n",
> Isn't it redundant to print PDU name here? […]
Yes, I'm printing it as debug after encoding it, this is helpful to find out that it was encoded successfuly.
--
To view, visit https://gerrit.osmocom.org/c/osmo-cbc/+/28819
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-cbc
Gerrit-Branch: master
Gerrit-Change-Id: Ifd1eca79bf4fac63de8066cd6a18004138d51d04
Gerrit-Change-Number: 28819
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 27 Jul 2022 18:44:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-cbc/+/28823 )
Change subject: sbcap: Store reported failed TAIs from WriteReplaceResponse in cbc_message_peer
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/osmo-cbc/+/28823
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-cbc
Gerrit-Branch: master
Gerrit-Change-Id: I37b08aa374c1097d2871bf69a7bb7893f32bccd3
Gerrit-Change-Number: 28823
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 27 Jul 2022 18:40:44 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment