Attention is currently required from: daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34144 )
Change subject: osmo_io: Avoid potential double free when sending msgb
......................................................................
Patch Set 1:
(3 comments)
Commit Message:
https://gerrit.osmocom.org/c/libosmocore/+/34144/comment/cd32b5e7_41a2bc70
PS1, Line 16: mean time. To avoid that set the parent of any msgb in the tx queue to the
time is indeed mean sometimes.
Patchset:
PS1:
All the talloc parent in here looks already quite complex and I have the feeling it's becoming even more complex here. I think all this deserves a bit more of explanation/documentation since I think I'm not gasping it completely.
IIUC we used to enqueue messages to be written to the socket in an internal queue of iofd, but those messages where actually not owned (talloc parent) by the iofd, but by some other random context outside of it. This looks wrong to me. messages enqueued in an iofd should always be owned by iofd.
I think you are going into this direction (iofd owning it) but the solution passing lots of extra ctx doesn't look good to me at first sight. I'd welcome further explanation on why all those ctx pointers are needed.
File src/core/osmo_io.c:
https://gerrit.osmocom.org/c/libosmocore/+/34144/comment/197b573b_3b9482a1
PS1, Line 324:
non-related ws
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34144
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I3a279b55a3adff96948120683c844e1508d0ba94
Gerrit-Change-Number: 34144
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 11 Aug 2023 16:32:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: neels, laforge, msuraev.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/32273 )
Change subject: add static SS7 routing example to cs7-config.adoc
......................................................................
Patch Set 3:
(1 comment)
File common/chapters/cs7-config.adoc:
https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/32273/comment/1d5251a7_b40d…
PS1, Line 365: Every component has a distinct M3UA port. For
> I don't immediately see a reason why the other side (BSC) would have to use a "distinct" port. […]
I also think this sentence is too generic or not accurate.
The problem here comes from that fact that we implement this using 1-to-1 SCTP sockets, which may pose some limitations on the IP addresses + local port that can be assigned in several different ASPs.
If we implemented 1-to-many sockets, we'd internally have 1 socket which means could be shared by several ASPs sharing the local addresses, and find out the ASP based on remote addresses. But since we are creating 1 socket per ASPs, the kernel doesn't allow re-creating another socket with the same IP address + local port (I think even if the local addresses are different, but I'd need to test it).
So maybe simply documenting this feature may be better than simply writing that the port must be unique.
--
To view, visit https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/32273
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-gsm-manuals
Gerrit-Branch: master
Gerrit-Change-Id: I44afddf7004f5bf37eec706ca3da12c04f83f8fa
Gerrit-Change-Number: 32273
Gerrit-PatchSet: 3
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: msuraev <msuraev(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: msuraev <msuraev(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 11 Aug 2023 16:21:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/34132
to look at the new patch set (#4).
Change subject: lapdm: Append RSL_IE_OSMO_ABS_FRAME_NUMBER to RSLms msgs towards upper layers
......................................................................
lapdm: Append RSL_IE_OSMO_ABS_FRAME_NUMBER to RSLms msgs towards upper layers
This makes it possible to track GSM time in the upper layers.
The existing RSL_IE_FRAME_NUMBER and RSL_IE_STARTNG_TIME cannot be used
there, since those are 16bit fields containing Relative FN values.
The IE needs to be added before the L3_INFO one, because user code
usually assumes the msgb->l3 pointing to L3_INFO value extends until the
end of the message, using msgb3_len(msg). Regarding having an extra IE
at the middle, it's not a big problem since the libosmocore version
submitting this commit to the upper layers is the same which will also
be parsing it through rsl_tlv_parse() later on by the app.
Related: OS#3626
Change-Id: Id62c18f49f270449067b25b7104eb8b47f1955ec
---
M include/osmocom/gsm/rsl.h
M src/gsm/lapdm.c
M src/gsm/rsl.c
M tests/lapd/lapd_test.c
M tests/lapd/lapd_test.ok
5 files changed, 74 insertions(+), 13 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/32/34132/4
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34132
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Id62c18f49f270449067b25b7104eb8b47f1955ec
Gerrit-Change-Number: 34132
Gerrit-PatchSet: 4
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34130 )
(
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: lapdm: Track fn of primitives in struct lapdm_msg_ctx
......................................................................
lapdm: Track fn of primitives in struct lapdm_msg_ctx
This field will be used in follow-up commits to provide FN information
in RSLms primitives towars upper layers. This is needed for instance on
the MS side when a CCCH_DATA.ind is received containing a TBF ImmAss
with a relative FN indicating the Starting Time. Without tracking FN
advance, the uppers layers are not capable of figuring out the absolute
FN of the TBF Starting time.
The struct lapdm_msg_ctx is not really used outside of libosmocore, so
we are safe extending it.
Related: OS#3626
Change-Id: Icf986f4202703eb452bedc1b749bb8ce0c73706f
---
M include/osmocom/gsm/lapdm.h
M src/gsm/lapdm.c
M tests/lapd/lapd_test.c
3 files changed, 26 insertions(+), 2 deletions(-)
Approvals:
laforge: Looks good to me, but someone else must approve
fixeria: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/include/osmocom/gsm/lapdm.h b/include/osmocom/gsm/lapdm.h
index 0e99743..1a39fca 100644
--- a/include/osmocom/gsm/lapdm.h
+++ b/include/osmocom/gsm/lapdm.h
@@ -24,6 +24,7 @@
uint8_t link_id;
uint8_t ta_ind; /* TA indicated by network */
uint8_t tx_power_ind; /* MS power indicated by network */
+ uint32_t fn;
};
/*! LAPDm datalink like TS 04.06 / Section 3.5.2 */
diff --git a/src/gsm/lapdm.c b/src/gsm/lapdm.c
index 4a82c4d..e96e218 100644
--- a/src/gsm/lapdm.c
+++ b/src/gsm/lapdm.c
@@ -700,7 +700,7 @@
/* input into layer2 (from layer 1) */
static int l2_ph_data_ind(struct msgb *msg, struct lapdm_entity *le,
- uint8_t chan_nr, uint8_t link_id)
+ uint8_t chan_nr, uint8_t link_id, uint32_t fn)
{
uint8_t cbits = chan_nr >> 3;
uint8_t sapi; /* we cannot take SAPI from link_id, as L1 has no clue */
@@ -715,6 +715,7 @@
memset(&mctx, 0, sizeof(mctx));
mctx.chan_nr = chan_nr;
mctx.link_id = link_id;
+ mctx.fn = fn;
/* check for L1 chan_nr/link_id and determine LAPDm hdr format */
if (cbits == 0x10 || cbits == 0x12) {
@@ -916,7 +917,7 @@
switch (OSMO_PRIM_HDR(oph)) {
case OSMO_PRIM(PRIM_PH_DATA, PRIM_OP_INDICATION):
rc = l2_ph_data_ind(oph->msg, le, pp->u.data.chan_nr,
- pp->u.data.link_id);
+ pp->u.data.link_id, pp->u.data.fn);
break;
case OSMO_PRIM(PRIM_PH_RTS, PRIM_OP_INDICATION):
rc = l2_ph_data_conf(oph->msg, le);
diff --git a/tests/lapd/lapd_test.c b/tests/lapd/lapd_test.c
index 739f9b7..2f2a7f4 100644
--- a/tests/lapd/lapd_test.c
+++ b/tests/lapd/lapd_test.c
@@ -184,6 +184,7 @@
/* LAPDm requires those... */
pp.u.data.chan_nr = 0;
pp.u.data.link_id = 0;
+ pp.u.data.fn = 0;
/* feed into the LAPDm code of libosmogsm */
rc = lapdm_phsap_up(&pp.oph, &chan->lapdm_dcch);
OSMO_ASSERT(rc == 0 || rc == -EBUSY);
@@ -206,6 +207,7 @@
/* LAPDm requires those... */
pp.u.data.chan_nr = 0;
pp.u.data.link_id = 0;
+ pp.u.data.fn = 0;
/* feed into the LAPDm code of libosmogsm */
rc = lapdm_phsap_up(&pp.oph, &chan->lapdm_dcch);
OSMO_ASSERT(rc == 0 || rc == -EBUSY);
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34130
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Icf986f4202703eb452bedc1b749bb8ce0c73706f
Gerrit-Change-Number: 34130
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33223 )
Change subject: l1gprs: implement TBF starting time support
......................................................................
Patch Set 8:
This change is ready for review.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33223
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I174e3c43d2f4c828a528710b284e62c9bb794122
Gerrit-Change-Number: 33223
Gerrit-PatchSet: 8
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 11 Aug 2023 16:06:29 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/34139 )
Change subject: trxcon: properly handle PDCH slotmask in UL/DL TBF CFG.Req
......................................................................
Patch Set 2:
(2 comments)
File include/l1gprs.h:
https://gerrit.osmocom.org/c/osmocom-bb/+/34139/comment/34a6e4f7_d792758a
PS2, Line 37: void *priv
> The `priv` is already accessible via `gprs->priv`, this argument can be removed.
I really prefer passing the priv pointer as a param. This is a totally usual way of providing callbacks, and it provides the user with immedatie concern that the priv pointer is there, no need to go check if it and how it can be accessed.
https://gerrit.osmocom.org/c/osmocom-bb/+/34139/comment/4f7e2ff3_34991bd9
PS2, Line 48: changed_cb
> It's currently way too generic: "something has changed, figure out what exactly yourself by checking […]
I'll look into this.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34139
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I4c2ff25217fba0b6b4704f023071b86ed9afb55c
Gerrit-Change-Number: 34139
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 11 Aug 2023 16:06:16 +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.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/34132
to look at the new patch set (#3).
Change subject: lapdm: Append RSL_IE_OSMO_ABS_FRAME_NUMBER to RSLms msgs towards upper layers
......................................................................
lapdm: Append RSL_IE_OSMO_ABS_FRAME_NUMBER to RSLms msgs towards upper layers
This makes it possible to track GSM time in the upper layers.
The existing RSL_IE_FRAME_NUMBER and RSL_IE_STARTNG_TIME cannot be used
there, since those are 16bit fields containing Relative FN values.
The IE needs to be added before the L3_INFO one, because user code
usually assumes the msgb->l3 pointing to L3_INFO value extends until the
end of the message, using msgb3_len(msg). Regarding having an extra IE
at the middle, it's not a big problem since the libosmocore version
submitting this commit to the upper layers is the same which will also
be parsing it through rsl_tlv_parse() later on by the app.
Related: OS#3626
Change-Id: Id62c18f49f270449067b25b7104eb8b47f1955ec
---
M include/osmocom/gsm/rsl.h
M src/gsm/lapdm.c
M src/gsm/rsl.c
M tests/lapd/lapd_test.c
M tests/lapd/lapd_test.ok
5 files changed, 74 insertions(+), 13 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/32/34132/3
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34132
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Id62c18f49f270449067b25b7104eb8b47f1955ec
Gerrit-Change-Number: 34132
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin.
Hello Jenkins Builder, laforge,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/34131
to look at the new patch set (#3).
Change subject: rsl: Introduce new osmocom extension IE RSL_IE_OSMO_ABS_FRAME_NUMBER
......................................................................
rsl: Introduce new osmocom extension IE RSL_IE_OSMO_ABS_FRAME_NUMBER
This will be used in RSLms to provide Absolute Frame Number information
of the primitive indications being sent to upper layers, so that it's
possible to track GSM time in the upper layers.
The existing RSL_IE_FRAME_NUMBER and RSL_IE_STARTNG_TIME cannot be used
there, since those are 16bit fields containing Relative FN values.
Related: OS#3626
Change-Id: Ia28caa24dd141b1162b6e11500d753353fe6500d
---
M include/osmocom/gsm/protocol/gsm_08_58.h
M src/gsm/rsl.c
2 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/31/34131/3
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34131
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ia28caa24dd141b1162b6e11500d753353fe6500d
Gerrit-Change-Number: 34131
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/34124 )
Change subject: tlv: Introduce API msgb_tv32_push()
......................................................................
tlv: Introduce API msgb_tv32_push()
msgb_tv32_put() already exists, but msgb_tv32_push doesn't.
The tv16 counterparts are already present, and having to pass 32bit
integers is also quite common, so let's add an API for it.
Change-Id: I68d5693a18d55ce8d0647359044157d7e5bfae50
---
M TODO-RELEASE
M include/osmocom/gsm/tlv.h
2 files changed, 24 insertions(+), 0 deletions(-)
Approvals:
laforge: Looks good to me, approved
fixeria: Looks good to me, but someone else must approve
Jenkins Builder: Verified
diff --git a/TODO-RELEASE b/TODO-RELEASE
index acd1a61..f78104e 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -17,3 +17,4 @@
libosmogsm MODIFY osmo_auth_impl callback function signature change. No known external users
libosmogsm ADD osmo_auth_c2
libosmogsm ADD OSMO_AUTH_ALG_TUAK
+libosmogsm ADD new API msgb_tv32_push()
\ No newline at end of file
diff --git a/include/osmocom/gsm/tlv.h b/include/osmocom/gsm/tlv.h
index fd6659c..28e897d 100644
--- a/include/osmocom/gsm/tlv.h
+++ b/include/osmocom/gsm/tlv.h
@@ -457,6 +457,16 @@
return buf;
}
+/*! push (prepend) a TV32 field to a \ref msgb
+ * \returns pointer to first byte of newly-pushed information */
+static inline uint8_t *msgb_tv32_push(struct msgb *msg, uint8_t tag, uint32_t val)
+{
+ uint8_t *buf = msgb_push(msg, 5);
+ *buf++ = tag;
+ osmo_store32be(val, buf);
+ return buf;
+}
+
/*! push (prepend) a TvLV field to a \ref msgb
* \returns pointer to first byte of newly-pushed information */
static inline uint8_t *msgb_tvlv_push(struct msgb *msg, uint8_t tag, uint16_t len,
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/34124
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I68d5693a18d55ce8d0647359044157d7e5bfae50
Gerrit-Change-Number: 34124
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged