July 2023 - gerrit-log - lists.osmocom.org

[S] Change in osmo-tetra[master]: Fixups and clarifying comments for msgb tail modifications
by wbokslag 29 Jul '23

29 Jul '23

Attention is currently required from: laforge. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-tetra/+/34000 to look at the new patch set (#3). Change subject: Fixups and clarifying comments for msgb tail modifications ...................................................................... Fixups and clarifying comments for msgb tail modifications Added fixups for msgb len field whenever the tail is modified Also, added some clarifying comments Change-Id: Ia725edbeafe26bd2ea9b5a1810d0b26bc79d84db --- M src/lower_mac/tetra_lower_mac.c M src/tetra_llc.c M src/tetra_upper_mac.c 3 files changed, 30 insertions(+), 9 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-tetra refs/changes/00/34000/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/34000 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-tetra Gerrit-Branch: master Gerrit-Change-Id: Ia725edbeafe26bd2ea9b5a1810d0b26bc79d84db Gerrit-Change-Number: 34000 Gerrit-PatchSet: 3 Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-tetra[master]: Added functions that prepare for decryption functionality
by wbokslag 29 Jul '23

29 Jul '23

Attention is currently required from: wbokslag. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-tetra/+/34001 to look at the new patch set (#3). Change subject: Added functions that prepare for decryption functionality ...................................................................... Added functions that prepare for decryption functionality Added functions that will eventually allow for mac resource decryption, voice/traffic decryption and identity decryption. Calls to the true TEA keystream generator functions and TA61 identity decryption function will need to be added in a later patch. Change-Id: I4e6147f206ad6046f32e08015ec9721b64382ca1 --- M src/crypto/tetra_crypto.c M src/crypto/tetra_crypto.h 2 files changed, 131 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-tetra refs/changes/01/34001/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/34001 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-tetra Gerrit-Branch: master Gerrit-Change-Id: I4e6147f206ad6046f32e08015ec9721b64382ca1 Gerrit-Change-Number: 34001 Gerrit-PatchSet: 3 Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl> Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-tetra[master]: Fixups and clarifying comments for msgb tail modifications
by wbokslag 29 Jul '23

29 Jul '23

Attention is currently required from: laforge. wbokslag has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/34000 ) Change subject: Fixups and clarifying comments for msgb tail modifications ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: After further inspection, I find the msgb_trim function uses msgb->data as a reference, which is a problem, due to the multiple-resources-per-timeslot scenario. It would work if l1h were used as a reference, since I move that pointer forward in tetra_lower_mac.c. I'm not against refactoring all the manual field manipulation to an osmocom buffer manipulation function but can you suggest a way to do it properly? What's the semantically correct way to deal with multiple upper mac messages in a buffer? File src/lower_mac/tetra_lower_mac.c: https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/173bdf49_1109459c PS2, Line 347: msg->len = msg > Okay, I appreciate the comments and effort to maintain good code quality. […] After further inspection, I find the msgb_trim function uses msgb->data as a reference, which is a problem, due to the multiple-resources-per-timeslot scenario. It would work if l1h were used as a reference, since I move that pointer forward in tetra_lower_mac.c. I'm not against refactoring all the manual field manipulation to an osmocom buffer manipulation function but can you suggest a way to do it properly? What's the semantically correct way to deal with multiple upper mac messages in a buffer? -- To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/34000 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-tetra Gerrit-Branch: master Gerrit-Change-Id: Ia725edbeafe26bd2ea9b5a1810d0b26bc79d84db Gerrit-Change-Number: 34000 Gerrit-PatchSet: 2 Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Sat, 29 Jul 2023 10:02:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: wbokslag <w.bokslag(a)midnightblue.nl> Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Gerrit-MessageType: comment

1 0

[S] Change in osmo-tetra[master]: Fixups and clarifying comments for msgb tail modifications
by wbokslag 29 Jul '23

29 Jul '23

Attention is currently required from: laforge. wbokslag has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/34000 ) Change subject: Fixups and clarifying comments for msgb tail modifications ...................................................................... Patch Set 2: (7 comments) Patchset: PS2: Hi, thanks again for the extensive comments. I elaborated the issue in a reply on your comment on tetra_lower_mac, and I believe indeed trim / l3trim may be suited for use in tetra_upper_mac and the LLC code File src/lower_mac/tetra_lower_mac.c: https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/c0575652_18ca0744 PS2, Line 347: msg->len = msg > in general we don't ever manually manipulate msgb's this way in other osmocom software. […] Okay, I appreciate the comments and effort to maintain good code quality. I am not deeply familiar with the osmocom primitives, so maybe I'm overlooking something. The essence of the problem is this. Suppose we have a timeslot on the signalling channel, which will contain MAC-RESOURCE pdus and the like. According to the standard, multiple PDUs may be contained in a single timeslot. So the MAC layer parsing code needs to have an accurate l1h, l2h, l3h start marker for the different layers of the protocol, BUT ALSO need to know how long the payload for their respective layer is. During parsing in tetra_upper_mac, the tail pointer is moved closer to the start, based on MAC-RESOURCE len field. Also, the tail pointer is moved closer to the start in the LLC layer (layer 2) if a 32-bit FCS is present AFTER the layer 3 MLE data. This is needed, because layer 3 parsing requires accurate start (l3h) and end/length markers. However, once the upper mac parsing is done and we return to the lower mac, we need to parse any further resources that may be present in the timeslot. That's why I advance the head, put the tail at the end of the slot again and then fix the end field. msgb_get seemed unsuitable for the purpose since its description states "removes data from the end", as such, I felt I have no guarantee the data there will remain intact and/or allocated. Also semantically it seems like we definitively discard this data. In your next comment you bring msgb_trim (for l2) and msgb_l3trim to my attention, that indeed looks suitable, I'll refactor. File src/tetra_llc.c: https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/b443b21b_ab95c3cf PS2, Line 124: msg->tail = msg->l3h + lpp.tl_sdu_len; // Strips off FCS (if present) : msg->len = msg->tail - msg->head; > this looks a bit like openn-coding a msgb_get(), or probably even more a msgb_l3trim() ? Done File src/tetra_upper_mac.c: https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/9bdada54_cca44860 PS2, Line 178: msg->len = m > same here. […] Done https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/e5e44e08_c7971e2a PS2, Line 185: msg->len = m > same here. […] Done https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/1b2939f2_78548fbe PS2, Line 306: msg->len = m > yet another msgb_trim? Also, the entire get_num_fill_bits + following operations happens several ti […] Ack https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/c818c925_1fb1ed55 PS2, Line 359: msg->len = m > again another fill-bits-stripping situation. Done -- To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/34000 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-tetra Gerrit-Branch: master Gerrit-Change-Id: Ia725edbeafe26bd2ea9b5a1810d0b26bc79d84db Gerrit-Change-Number: 34000 Gerrit-PatchSet: 2 Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Sat, 29 Jul 2023 09:47:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Gerrit-MessageType: comment

1 0

[S] Change in pysim[master]: README.md: Add note about pySim-trace.py dependencies
by laforge 29 Jul '23

29 Jul '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/pysim/+/33994 ) Change subject: README.md: Add note about pySim-trace.py dependencies ...................................................................... README.md: Add note about pySim-trace.py dependencies Related: OS#6094 Change-Id: I2e03f9c827bd6ee73891bba34bd2f2efe3ded7e6 --- M README.md 1 file changed, 13 insertions(+), 0 deletions(-) Approvals: laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/README.md b/README.md index 2a89c9a..c3c5a25 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,9 @@ After installing all dependencies, the pySim applications ``pySim-read.py``, ``pySim-prog.py`` and ``pySim-shell.py`` may be started directly from the cloned repository. +In addition to the dependencies above ``pySim-trace.py`` requires ``tshark`` and the python package ``pyshark`` to be installed. It is known that the ``tshark`` package +in Debian versions before 11 may not work with pyshark. + ### Archlinux Package Archlinux users may install the package ``python-pysim-git`` -- To view, visit https://gerrit.osmocom.org/c/pysim/+/33994 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I2e03f9c827bd6ee73891bba34bd2f2efe3ded7e6 Gerrit-Change-Number: 33994 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in pysim[master]: pysim-test: rename pysim-test.sh to pySim-prog_test.sh
by laforge 29 Jul '23

29 Jul '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/pysim/+/33964 ) ( 2 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: pysim-test: rename pysim-test.sh to pySim-prog_test.sh ...................................................................... pysim-test: rename pysim-test.sh to pySim-prog_test.sh We now have pySim-shell and pySim-trace. Let's give pysim-test.sh a more distinctive name so that it is clear to which program it refers. Related: OS#6094 Change-Id: I438f63f9580ebd3c7cc78cc5dab13c9937ac6e3a --- M contrib/jenkins.sh R tests/pySim-prog_test.sh 2 files changed, 16 insertions(+), 3 deletions(-) Approvals: laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, approved Jenkins Builder: Verified diff --git a/contrib/jenkins.sh b/contrib/jenkins.sh index 3e02546..e75ac40 100755 --- a/contrib/jenkins.sh +++ b/contrib/jenkins.sh @@ -29,7 +29,7 @@ # Run the test with physical cards cd pysim-testdata - ../tests/pysim-test.sh + ../tests/pySim-prog_test.sh ../tests/pySim-trace_test.sh ;; "pylint") diff --git a/tests/pysim-test.sh b/tests/pySim-prog_test.sh similarity index 97% rename from tests/pysim-test.sh rename to tests/pySim-prog_test.sh index 4004c18..f248768 100755 --- a/tests/pysim-test.sh +++ b/tests/pySim-prog_test.sh @@ -27,8 +27,8 @@ set -e -echo "pysim-test - a test program to test pysim-prog.py" -echo "=================================================" +echo "pySim-prog_test - a test program to test pysim-prog.py" +echo "======================================================" # Generate a list of the cards we expect to see by checking which .ok files # are present -- To view, visit https://gerrit.osmocom.org/c/pysim/+/33964 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I438f63f9580ebd3c7cc78cc5dab13c9937ac6e3a Gerrit-Change-Number: 33964 Gerrit-PatchSet: 5 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in pysim[master]: tests: add test script for pySim-trace
by laforge 29 Jul '23

29 Jul '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/pysim/+/33963 ) Change subject: tests: add test script for pySim-trace ...................................................................... tests: add test script for pySim-trace pySim-trace has no test coverage yet. Let's use a script to run a GSAMTAP pcacp through it and check that no exceptions are raised. Related: OS#6094 Change-Id: Icfabfa7c59968021eef0399991bd05b92467d8d2 --- M contrib/jenkins.sh A pysim-testdata/pySim-trace_test_gsmtap.pcapng A tests/pySim-trace_test.sh 3 files changed, 35 insertions(+), 0 deletions(-) Approvals: laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/contrib/jenkins.sh b/contrib/jenkins.sh index 0309877..3e02546 100755 --- a/contrib/jenkins.sh +++ b/contrib/jenkins.sh @@ -22,6 +22,7 @@ . venv/bin/activate pip install -r requirements.txt + pip install pyshark # Execute automatically discovered unit tests first python -m unittest discover -v -s tests/ @@ -29,6 +30,7 @@ # Run the test with physical cards cd pysim-testdata ../tests/pysim-test.sh + ../tests/pySim-trace_test.sh ;; "pylint") # Print pylint version diff --git a/pysim-testdata/pySim-trace_test_gsmtap.pcapng b/pysim-testdata/pySim-trace_test_gsmtap.pcapng new file mode 100644 index 0000000..35c3003 --- /dev/null +++ b/pysim-testdata/pySim-trace_test_gsmtap.pcapng Binary files differ diff --git a/tests/pySim-trace_test.sh b/tests/pySim-trace_test.sh new file mode 100755 index 0000000..0ec5e0f --- /dev/null +++ b/tests/pySim-trace_test.sh @@ -0,0 +1,20 @@ +#/bin/bash + +PYSIM_TRACE=../pySim-trace.py +GSMTAP_TRACE=pySim-trace_test_gsmtap.pcapng + +echo "pySim-trace_test - a test program to test pySim-trace.py" +echo "========================================================" + +$PYSIM_TRACE gsmtap-pyshark-pcap -f $GSMTAP_TRACE +if [ $? -ne 0 ]; then + echo "" + echo "========================================================" + echo "Testrun with $GSMTAP_TRACE failed." + exit 1 +fi + +echo "" +echo "========================================================" +echo "trace parsed without problems -- everything ok!" + -- To view, visit https://gerrit.osmocom.org/c/pysim/+/33963 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: Icfabfa7c59968021eef0399991bd05b92467d8d2 Gerrit-Change-Number: 33963 Gerrit-PatchSet: 5 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in pysim[master]: tests: add test script for pySim-trace
by laforge 29 Jul '23

29 Jul '23

Attention is currently required from: dexter. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33963 ) Change subject: tests: add test script for pySim-trace ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/pysim/+/33963 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: Icfabfa7c59968021eef0399991bd05b92467d8d2 Gerrit-Change-Number: 33963 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 29 Jul 2023 08:56:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in pysim[master]: README.md: Add note about pySim-trace.py dependencies
by laforge 29 Jul '23

29 Jul '23

Attention is currently required from: dexter. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33994 ) Change subject: README.md: Add note about pySim-trace.py dependencies ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/pysim/+/33994 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I2e03f9c827bd6ee73891bba34bd2f2efe3ded7e6 Gerrit-Change-Number: 33994 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sat, 29 Jul 2023 08:55:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-ci[master]: jobs/master,gerrit: don't use bullseye for python2
by laforge 29 Jul '23

29 Jul '23

Attention is currently required from: osmith. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ci/+/33993 ) Change subject: jobs/master,gerrit: don't use bullseye for python2 ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-ci/+/33993 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ci Gerrit-Branch: master Gerrit-Change-Id: I1d9204b5b59866fa79839221bb47ec4f7206a982 Gerrit-Change-Number: 33993 Gerrit-PatchSet: 1 Gerrit-Owner: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Comment-Date: Sat, 29 Jul 2023 08:55:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0