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=ema... : 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=ema... : 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=ema... : 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=ema... : 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=ema... : 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=ema... : 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=ema... : 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=ema... : 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=ema... : 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 <pmaier@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Attention: fixeria <vyanitskiy@sysmocom.de> Gerrit-Comment-Date: Mon, 31 Mar 2025 15:00:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge@osmocom.org> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> Comment-In-Reply-To: dexter <pmaier@sysmocom.de>