[L] Change in pysim[master]: saip-tool: add features to add, remove and inspect application PEs - gerrit-log

31 Mar 2025

Attention is currently required from: fixeria, laforge.

dexter has posted comments on this change by dexter. (
https://gerrit.osmocom.org/c/pysim/+/39860?usp=email )

Change subject: saip-tool: add features to add, remove and inspect application PEs
......................................................................

Patch Set 2:

(9 comments)

File contrib/saip-tool.py:

https://gerrit.osmocom.org/c/pysim/+/39860/comment/102e8d2a_e2431590?usp=em… :
PS1, Line 22: import zipfile
...
  Acknowledged Yes and no. I have split the
patchset up now to make it easier to read.

https://gerrit.osmocom.org/c/pysim/+/39860/comment/826493e5_ecb72e93?usp=em… :
PS1, Line 250: if dictionary is None:
...
  The type hint `:dict` suggests that it cannot be
`None`, change to `Optional[dict]` then? Done

https://gerrit.osmocom.org/c/pysim/+/39860/comment/f8c71f2c_c22570f8?usp=em… :
PS1, Line 255:         elif value is None:
             :   
...
  Looks like a no-op branch that can be removed without
any consequences? Not really, but there is a bug. If value is none, then we must not
try to hexdump value. I forgot a return statement. Maybe it is more clear now.

https://gerrit.osmocom.org/c/pysim/+/39860/comment/40ee142e_85ccb2c8?usp=em… :
PS1, Line 267:     app_index = 0
...
  shouldn't we simply make this part of the
'info' command? We already display some basic information  […] When I was
using the list-apps sub-command I was happy to see only the application related output and
not any additional information. So I think that it is a good idea to have a --apps
parameter that only prints the application related output while hiding the basic info.
This is also good when we add more PE specific info commands in the future.

https://gerrit.osmocom.org/c/pysim/+/39860/comment/e35c7a25_1b717df5?usp=em… :
PS1, Line 268: for app_pe in apps:
...
  You can drop `app_index` above and use `enumerate()`
instead: […] Thanks, I didn't know that before.

https://gerrit.osmocom.org/c/pysim/+/39860/comment/3c7dc52e_830efa3b?usp=em… :
PS1, Line 272:   
...
  I think this line should be removed, it's likely
just a debug aid? Done

https://gerrit.osmocom.org/c/pysim/+/39860/comment/8c97c1dd_cc6a4211?usp=em… :
PS1, Line 283: for inst in app_pe.decoded.get('instanceList', []):
...
  Likewise, use `enumerate()` here. Done

https://gerrit.osmocom.org/c/pysim/+/39860/comment/e02a5c24_650ff460?usp=em… :
PS1, Line 349: 0]
...
  There can be multiple security domains.  It's
probably unusual, but legal AFAIR. […] Done

https://gerrit.osmocom.org/c/pysim/+/39860/comment/f7ac1421_26b1e98c?usp=em… :
PS1, Line 375: 
...
  yes, I think a method would be better style.
Done

-- 
To view, visit https://gerrit.osmocom.org/c/pysim/+/39860?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: I41db96f2f0ccc29c1725a92215ce6b17d87b76ce
Gerrit-Change-Number: 39860
Gerrit-PatchSet: 2
Gerrit-Owner: dexter &lt;pmaier(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-CC: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Comment-Date: Mon, 31 Mar 2025 15:00:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt;
Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Comment-In-Reply-To: dexter &lt;pmaier(a)sysmocom.de&gt;