Attention is currently required from: dexter, laforge. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/36930?usp=email ) Change subject: CardKeyProvider: Implement support for column-based transport key encryption ...................................................................... Patch Set 1: Code-Review+1 (7 comments) Patchset: PS1: None of my comments are merge blockers, mostly cosmetics. File contrib/csv-encrypt-columns.py: https://gerrit.osmocom.org/c/pysim/+/36930/comment/23e659d5_7fcba862 PS1, Line 29: def dict_keys_to_upper(d: dict): typing hint: `-> dict:` https://gerrit.osmocom.org/c/pysim/+/36930/comment/422e01a1_fe6d086e PS1, Line 37: def encrypt_col(self, colname:str, value: str): typing hint: `-> Hexstr:`? https://gerrit.osmocom.org/c/pysim/+/36930/comment/b16674bf_85935630 PS1, Line 43: def encrypt(self): type hint: `-> None:` https://gerrit.osmocom.org/c/pysim/+/36930/comment/561b5356_52ed67f2 PS1, Line 60: parser.add_argument('CSVFILE', help="CSV file name") Idea: you can do `type=argparse.FileType('r')`, so that argparse will `open()` the file for you and fail early if it does not exist. Not sure when it gets `close()d` though (perhaps automatically when the script finishes?). As a bonus, a special value `-` will automatically open `stdin`, but in that case the `self.filename + '.encr` logic above will no longer work. Maybe generalize this even further, allowing to specify the output file and using `stdin` / `stdout` by default? ``` parser.add_argument('-i', metavar='FILE', type=argparse.FileType('r'), default=sys.stdin, help="Input CSV file to be encrypted") parser.add_argument('-o', metavar='FILE', type=argparse.FileType('w'), default=sys.stdout, help="Output CSV file") ``` https://gerrit.osmocom.org/c/pysim/+/36930/comment/11e2dd34_48cfb2c5 PS1, Line 61: default=[], I would remove `default` and put `required=True`, since you require at least one argument. File pySim/card_key_provider.py: https://gerrit.osmocom.org/c/pysim/+/36930/comment/4e88736f_0a5f7721 PS1, Line 129: str) -> str `Hexstr`? -- To view, visit https://gerrit.osmocom.org/c/pysim/+/36930?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I13146a799448d03c681dc868aaa31eb78b7821ff Gerrit-Change-Number: 36930 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Mon, 27 May 2024 20:10:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment