laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33347 )
Change subject: Add support for encoding/decoding SMS in TPDU and SMPP format
......................................................................
Set Ready For Review
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/33347
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I0d95e62c1e7183a7851d1fe38df0f5133830cb1f
Gerrit-Change-Number: 33347
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 18 Jun 2023 07:58:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: laforge.
Jenkins Builder has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33348 )
Change subject: tests: Add new, data-driven OTA tests
......................................................................
Patch Set 2:
(1 comment)
File tests/test_ota.py:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-8398):
https://gerrit.osmocom.org/c/pysim/+/33348/comment/fb331739_b733594c
PS2, Line 181: # unittest subTests context manager to iterate over the entires of
'entires' may be misspelled - perhaps 'entries'?
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/33348
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I8789a21fa5a4793bdabd468adc9fee3b6e633c25
Gerrit-Change-Number: 33348
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Sun, 18 Jun 2023 07:15:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: arehbein, pespin, daniel.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33083 )
Change subject: gsm/ipa: Add segmentation callback
......................................................................
Patch Set 10: Code-Review-1
(7 comments)
File src/gsm/ipa.c:
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/854abcf7_d002ad5c
PS8, Line 728: * -EIO, if the header declares a payload too large */
> */ on the next line
Done
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/3a4ea39a_1c162155
PS8, Line 735: const struct ipaccess_head *hh = (const struct ipaccess_head *) msg->data;
> Agreeing with Pau here. […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/33e71ea9_d53bedf9
PS8, Line 737: size_t total_len = payload_len + sizeof(*hh);
> "sizeof(*hh) + payload_len;" it's logically easier to understand, as in lefto-to-right order filling […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/821e589a_d23d9a03
PS8, Line 738: if (msgb_tailroom(msg) + msgb_length(msg) < total_len) {
> > iiuc the problem here is that the allocated msgb space is not going to be enough to fit in what IP […]
Done
File src/gsm/ipa.c:
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/441ff0d5_56f4edf9
PS10, Line 733: osmo_ntohs(hh->len);
Now the problem is that you're accessing the buffer before checking if there is enough data in it.
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/d6b63790_c2f79979
PS10, Line 735: msgb_length(msg) + msgb_tailroom(msg);
This is incorrect. `msgb_length(msg)` is basically `msg->len`, which does include the tailroom and headroom. Please revert back to `msg->len` or `msgb_length(msg)` alone.
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/bcbc3027_8e493c26
PS10, Line 744: EIO
`ENOMEM` or `ENOSPC` is a better fit here, IMO.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33083
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I3a639e6896cc3b3fc8e9b2e1a58254710efa0d3f
Gerrit-Change-Number: 33083
Gerrit-PatchSet: 10
Gerrit-Owner: arehbein <arehbein(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-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 17 Jun 2023 19:05:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: laforge.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33346 )
Change subject: core/osmo_io: Fix reception of partial packets
......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
> I don't really understand this patch (but then I'm not an expert in the new osmo_io code). […]
You're correct, I didn't at first understand the code well enough and thought of maintaining `iofd->pending` parallel to `msg` (in `iofd_poll_ofd_cb_recvmsg_sendmsg()`) in case of an incompletely received message (instead of removing it from the osmo_io fd via ` msg = iofd_msgb_pending_or_alloc(iofd);`).
So my first approach was to maintain the old logic while trying to add new logic in case of having previously received an incompletely read packet, which is why it turned out like this.
File src/core/osmo_io_poll.c:
https://gerrit.osmocom.org/c/libosmocore/+/33346/comment/411d08d3_9fb8bbe9
PS1, Line 59: iofd
> IMHO, the unconditional iov_base = msg->tail should do the trick here. […]
Yes thanks, good catch. I changed the patch accordingly.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33346
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I0ab8028c807b4181fddd3c00ea2e037c40cf0813
Gerrit-Change-Number: 33346
Gerrit-PatchSet: 1
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Sat, 17 Jun 2023 18:40:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
Hello Jenkins Builder, laforge,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/33346
to look at the new patch set (#2).
Change subject: core/osmo_io: Fix reception of partial packets
......................................................................
core/osmo_io: Fix reception of partial packets
Always append to 'msg->tail' instead of to 'msg->data'.
Change-Id: I0ab8028c807b4181fddd3c00ea2e037c40cf0813
---
M src/core/osmo_io_poll.c
1 file changed, 12 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/46/33346/2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33346
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I0ab8028c807b4181fddd3c00ea2e037c40cf0813
Gerrit-Change-Number: 33346
Gerrit-PatchSet: 2
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: arehbein, pespin, daniel.
Hello Jenkins Builder, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/33083
to look at the new patch set (#10).
Change subject: gsm/ipa: Add segmentation callback
......................................................................
gsm/ipa: Add segmentation callback
Add segmentation callback to be used by the streaming backend of libosmo-netif
Related: OS#5753, OS#5751
Change-Id: I3a639e6896cc3b3fc8e9b2e1a58254710efa0d3f
---
M include/osmocom/gsm/ipa.h
M src/gsm/ipa.c
M src/gsm/libosmogsm.map
3 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/83/33083/10
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33083
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I3a639e6896cc3b3fc8e9b2e1a58254710efa0d3f
Gerrit-Change-Number: 33083
Gerrit-PatchSet: 10
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: laforge.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33347 )
Change subject: Add support for encoding/decoding SMS in TPDU and SMPP format
......................................................................
Patch Set 1:
(2 comments)
File contrib/jenkins.sh:
https://gerrit.osmocom.org/c/pysim/+/33347/comment/5d4b969b_312879de
PS1, Line 25: pip install git+https://github.com/hologram-io/smpp.pdu
Is this really needed given that you adding this dep to `requirements.txt`?
File ota_test.py:
https://gerrit.osmocom.org/c/pysim/+/33347/comment/8ab8e82e_5143a312
PS1, Line 1: #!/usr
`/usr/bin/env python3` please
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/33347
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I0d95e62c1e7183a7851d1fe38df0f5133830cb1f
Gerrit-Change-Number: 33347
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Sat, 17 Jun 2023 11:25:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Jenkins Builder has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33348 )
Change subject: tests: Add new, data-driven OTA tests
......................................................................
Patch Set 1:
(1 comment)
File tests/test_ota.py:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-8395):
https://gerrit.osmocom.org/c/pysim/+/33348/comment/da54181a_4572c9b9
PS1, Line 181: # unittest subTests context manager to iterate over the entires of
'entires' may be misspelled - perhaps 'entries'?
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/33348
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I8789a21fa5a4793bdabd468adc9fee3b6e633c25
Gerrit-Change-Number: 33348
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-CC: Jenkins Builder
Gerrit-Comment-Date: Sat, 17 Jun 2023 11:11:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment