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