Attention is currently required from: fixeria, laforge. neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/pysim/+/40208?usp=email )
Change subject: personalization: implement UppAudit and BatchAudit ...................................................................... Patch Set 11: (1 comment) File pySim/esim/saip/batch.py: https://gerrit.osmocom.org/c/pysim/+/40208/comment/2682c181_b25146ad?usp=email : PS11, Line 143: {'001010000000023'} > Just to clarify: I think it's fine to not support multiple USIM NAA and/or > multipel ISIM NAA for now […] I follow your conclusions, but there is one initial assumption that I'd like to clarify: "our code should be safe. So if we do not support multiple USIM NAAs or multiple ISIM NAAs from the audit code (or any other part of the code), it should safely abort and refuse to proceed with such a PESequence as input data" The audit code is safe in this way, because it does support multiple occurences. Use cases in pseudo code: UPP{ usim: {imsi: 1234} } --audit--> {IMSI: {1234}} UPP{ usim: {imsi: 1234}, isim: {imsi: 1234} } --audit--> {IMSI: {1234}} UPP{ usim: {imsi: 1234}, isim: {imsi: 1234}, usim2: {imsi: 5678}, isim2: {imsi: 5678}} --audit--> {IMSI: {1234, 5678}} (caller decides about expecting two IMSIs or not) So code that is handling multiple USIM NAAs would expect both IMSIs in the resulting set. The audit would not indicate which IMSI is where exactly, but the aim of BatchAudit is to simplify. If we add an exception as discussed, the last example would not return any result but raise an exception saying "multiple USIM NAAs are not supported". That change would reduce current usefulness, and the code raising the exception would also be a side island of code that has to specifically detect that situation -- it would not arise naturally from the code paths. So, we are now deciding in this patch, to which level of detail the audit result returns its info. It's at this point not about preventing crashes from situations we don't support, because i expect no crashes in any use cases. I can understand that it may be useful to get an indication of where specific data was read from. Currently there is no caller that needs to know this, so I am merely guessing about use cases you have in mind. (IOW, all current callers would ignore the source indication and would always collapse to a single set() before evaluating.) So, what is the next thing that should happen in this patch? IIUC I can make a decision to leave the patch as-is, or to change the set() of values to a dict that has a place to indicate the source of the data? Instead of inventing an xpath like syntax now, I would just put an empty placeholder for future expansion, because there are currently no use cases for it -- so that we can implement a path syntax when we know what exactly the place indication needs to indicate in order to be useful. It may boil down to only a comment: "callers must check via isinstance() whether the result is a set() or a dict()", just to open up a path to future expansion. In summary, I am unfortunately still convinced that the way the current patch returns multiple occurences is a good simplification that matches the use cases well -- "unfortunately" because I am trying to agree in order to reach a conclusion but the logic doesn't match up from my POV. (BTW the argument I make about "this code is already used in production" is more like: "i've already played through all code paths end-to-end such that i have a clear picture of what we need this code to do, and thus i found what i believe is the optimal balance of detail and simplicity. The way this works has helped super well in various situations. I've found and fixed numerous UPP bugs with this: it is stable and useful. If i wrote it from scratch i would end up with exactly the same patch." I am very happy to completely overhaul this code, and then adjust the production code -- if there is an actual use case that i missed.) -- To view, visit https://gerrit.osmocom.org/c/pysim/+/40208?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: Iaab336ca91b483ecdddd5c6c8e08dc475dc6bd0a Gerrit-Change-Number: 40208 Gerrit-PatchSet: 11 Gerrit-Owner: neels <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Thu, 18 Jun 2026 13:28:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: laforge <[email protected]>
