Attention is currently required from: dexter. laforge has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/pysim/+/38195?usp=email ) Change subject: filesystem: pass total_len to construct of when encoding file contents ...................................................................... Patch Set 1: Code-Review-1 (4 comments) File pySim/filesystem.py: https://gerrit.osmocom.org/c/pysim/+/38195/comment/de34e383_98a0ce51?usp=em… : PS1, Line 182: ' unrelated cosmetic change. Not critical, but in case you are doing another version of this patch anyway, this should be removed. https://gerrit.osmocom.org/c/pysim/+/38195/comment/336e860e_8ae0f3e4?usp=em… : PS1, Line 747: if total_len is not None: this could deserve some documentation (doc-string or comment). For somebody not super familiar with every bit of the code (I guess everyone ecxcept me and now you) the functionality will not be obvious. You could also do it like this ``` if total_len is not None: # caller has provided the on-card size ... elif self.size[1] is not None: # use the recommended size ... elif self.size[0] is not None: # use the minimum size ``` btw: I'm not entirely sure if self.size could be None, at whcih point self.size[1] or [0] would cause an exception. https://gerrit.osmocom.org/c/pysim/+/38195/comment/839cddea_3246df69?usp=em… : PS1, Line 1043: __get_rec_len why do we have double-underscores here but single-underscore for _get_size() above? Also: please add comments/documentation like for _get_size, as I requested above. https://gerrit.osmocom.org/c/pysim/+/38195/comment/c4c2b572_564fe968?usp=em… : PS1, Line 1266: return b2h(filter_dict(build_construct(self._construct, abstract_data, self._get_size(total_len)))) Did you test that? I think a _get_rec_len() would actually be required here. Calling _get_size() of the parent class (TransparentEF) will return the total file size, ad not the size of an individual record. The TransRecEf.encode_record_{hex,bin} should only include a single record. The TransRecEF._encode_bin() below then iterates over all records. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/38195?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: I1b7a51594fbc5d9fe01132c39354a2fa88d53f9b Gerrit-Change-Number: 38195 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Tue, 17 Sep 2024 18:25:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes