Attention is currently required from: Hoernchen.
dexter has posted comments on this change by Hoernchen. ( https://gerrit.osmocom.org/c/pysim/+/43544?usp=email )
Change subject: sms/smpp-ota-tool: reassemble multi part response SMS ......................................................................
Patch Set 2: Code-Review+1
(7 comments)
Patchset:
PS2: I think this patch should be ok, also there are comprehenive testcases which show that it works. But as far as I can see it is possible to send incomplete message sets, which then pile up in the ConcatenatedSmsReassembler. This may be a problem for long running instances. This is something you should check back.
File pySim/sms.py:
https://gerrit.osmocom.org/c/pysim/+/43544/comment/e78dd65e_4450d2b0?usp=ema... : PS2, Line 99: # TS 23.040 9.2.3.24.1/.8 IEI of the concat IE You are referencing to 9.2.3.24.1 and 9.2.3.24.8, this is 100% correct, but It sent me a bit off track when I wanted to lookup the IEI hex values. Those are in the table at 9.2.3.24
I would recommend to put the reference like so:
``` # IEI: Concatenated short messages, 8-bit reference number # (see 3GPP TS 23.040 section 9.2.3.24 and section 9.2.3.24.1) CONCAT_8BIT = 0x00
# IEI: Concatenated short message, 16-bit reference number # (see 3GPP TS 23.040 section 9.2.3.24 and section 9.2.3.24.8) CONCAT_16BIT = 0x08
```
https://gerrit.osmocom.org/c/pysim/+/43544/comment/73fd2e5d_07688ec7?usp=ema... : PS2, Line 132: # TS 23.040 9.2.3.24.1.8 , total zero or seqno zero / > total: Did you mean TS 23.040 9.2.3.24.1/8 ?
https://gerrit.osmocom.org/c/pysim/+/43544/comment/0a8acfcc_f6caee7d?usp=ema... : PS2, Line 153: if len(s['parts']) < total: I wonder what happens when someone keeps sending incomplete sets. For example a set of three messages is expected, but only two arrive. Then the incomplete set is stuck forever, creating a memory leak.
I don't know if this is an issue or not, but as far as I understand, the API user creates an ConcatenatedSmsReassembler object once and then passes all incoming messages through it. The object exists as long as the process runs.
If this is a problem I would suggest to put a timestamp when the set is created. When the .add method is called, the sets are checked for expired timestamps. This way incomplete sets can never pile up over time.
File tests/unittests/test_sms.py:
https://gerrit.osmocom.org/c/pysim/+/43544/comment/ab35adfa_9b02834f?usp=ema... : PS2, Line 109: """TS 23.040 9.2.3.24 reassembly of multi-part SMS. I would recommend to reference the spac as "3GPP TS 23.040" and not only "TS ..."
(sure, you could look at the dots and the number of digits, but I think its better to be verbose)
https://gerrit.osmocom.org/c/pysim/+/43544/comment/6c369a12_667cd48c?usp=ema... : PS2, Line 113: form before decoding.""" I would rephrase it a bit:
An OTA response that exceeds a single SHORT MESSAGE is delivered in several parts using the SEND SHORT MESSAGE proactive command. The receiver must recombine the individual parts into a single part before decoding.
https://gerrit.osmocom.org/c/pysim/+/43544/comment/d50a58dc_0767571a?usp=ema... : PS2, Line 137: self.assertEqual(b2h(built), '0700030102017100') maybe move test_ground_truth_udh above test_single_part_passthrough? It seems to be just a precursor that verifies that the _concat methods work correctly. How about a similar test to test _concat16 as well?
-
dexter