dexter has uploaded this change for review. ( https://gerrit.osmocom.org/c/pysim/+/34932?usp=email ) Change subject: runtime: refactor file selection methods select and select_file ...................................................................... runtime: refactor file selection methods select and select_file The implementation of the methods select and select_file of class RuntimeLchan is a bit complex. We access the card directly in several places which makes it difficult to track the state changes. We should clean this up so that we call self.rs.card.select_adf_by_aid/ self.scc.select_file from a single place only. This means that the method select uses the method select_file. This results in a much cleaner implementation. We also should take care that the important states that we track (selected_file, selected_adf, etc.) are updated by a single private method. Since the update always must happen after a select _select_post is a good place to do this. Related: OS#6210 Change-Id: I9ae213f3b078983f3e6d4c11db38fdbe504c84f2 --- M pySim/runtime.py 1 file changed, 89 insertions(+), 41 deletions(-) git pull ssh://gerrit.osmocom.org:29418/pysim refs/changes/32/34932/1 diff --git a/pySim/runtime.py b/pySim/runtime.py index 8660724..feaa355 100644 --- a/pySim/runtime.py +++ b/pySim/runtime.py @@ -64,7 +64,7 @@ self.mf.add_file(f) # go back to MF before the next steps (addon probing might have changed DF) - self.card._scc.select_file('3F00') + self.lchan[0].select('3F00') # add application ADFs + MF-files from profile apps = self._match_applications() @@ -110,6 +110,12 @@ # probe for those applications for f in sorted(set(apps_profile) - set(apps_taken), key=str): try: + # we can not use the lchan provided methods select, or select_file + # since those method work on an already finished file model. At + # this point we are still in the initialization process, so it is + # no problem when we access the card directly without caring about + # updating other states. For normal selects at runtime, the caller + # must use the lchan provided methods select or select_file! data, sw = self.card.select_adf_by_aid(f.aid) if sw == "9000": print(" %s: %s" % (f.name, f.aid)) @@ -163,11 +169,14 @@ def __init__(self, lchan_nr: int, rs: RuntimeState): self.lchan_nr = lchan_nr self.rs = rs + self.scc = self.rs.card._scc.fork_lchan(lchan_nr) + + # File reference data self.selected_file = self.rs.mf self.selected_adf = None self.selected_file_fcp = None self.selected_file_fcp_hex = None - self.scc = self.rs.card._scc.fork_lchan(lchan_nr) + def add_lchan(self, lchan_nr: int) -> 'RuntimeLchan': """Add a new logical channel from the current logical channel. Just affects @@ -246,9 +255,17 @@ raise ValueError( "Cannot select unknown file by name %s, only hexadecimal 4 digit FID is allowed" % fid) + self._select_pre(cmd_app) + try: + # We access the card through the select_file method of the scc object. + # If we succeed, we know that the file exists and we may proceed with + # creating a new file at run time. In case the file does not exist, we + # just abort. The state on the card (selected file/application) wont't + # be changed, so we do not have to update any state in that case. (data, sw) = self.scc.select_file(fid) except SwMatchError as swm: + self._select_post(cmd_app) k = self.interpret_sw(swm.sw_actual) if not k: raise(swm) @@ -267,8 +284,8 @@ desc="elementary file, manually added at runtime") self.selected_file.add_files([f]) - self.selected_file = f - return select_resp, data + + self._select_post(cmd_app, f, data) def _select_pre(self, cmd_app): # unregister commands of old file @@ -276,7 +293,17 @@ for c in self.selected_file.shell_commands: cmd_app.unregister_command_set(c) - def _select_post(self, cmd_app): + def _select_post(self, cmd_app, file:CardFile = None, select_resp_data = None): + # we store some reference data (see above) about the currently selected file. + # This data must be updated after every select. + if file: + self.selected_file = file + if isinstance(file, CardADF): + self.selected_adf = file + if select_resp_data: + self.selected_file_fcp_hex = select_resp_data + self.selected_file_fcp = self.selected_file.decode_select_response(select_resp_data) + # register commands of new file if cmd_app and self.selected_file.shell_commands: for c in self.selected_file.shell_commands: @@ -293,22 +320,31 @@ inter_path = self.selected_file.build_select_path_to(file) if not inter_path: raise RuntimeError('Cannot determine path from %s to %s' % (self.selected_file, file)) - self._select_pre(cmd_app) - for p in inter_path: + # be sure the variables that we pass to _select_post contain valid values. + selected_file = self.selected_file + data = self.selected_file_fcp_hex + + for f in inter_path: try: - if isinstance(p, CardADF): - (data, sw) = self.rs.card.select_adf_by_aid(p.aid, scc=self.scc) - self.selected_adf = p + # We now directly accessing the card to perform the selection. This + # will change the state of the card, so we must take care to update + # the local state (lchan) as well. This is done in the method + # _select_port. It should be noted that the caller must always use + # the methods select_file or select. The caller must not access the + # card directly since this would lead into an incoherence of the + # card state and the state of the lchan. + if isinstance(f, CardADF): + (data, sw) = self.rs.card.select_adf_by_aid(f.aid, scc=self.scc) else: - (data, sw) = self.scc.select_file(p.fid) - self.selected_file = p + (data, sw) = self.scc.select_file(f.fid) + selected_file = f except SwMatchError as swm: - self._select_post(cmd_app) + self._select_post(cmd_app, selected_file, data) raise(swm) - self._select_post(cmd_app) + self._select_post(cmd_app, f, data) def select(self, name: str, cmd_app=None): """Select a file (EF, DF, ADF, MF, ...). @@ -317,9 +353,11 @@ name : Name of file to select cmd_app : Command Application State (for unregistering old file commands) """ + # if any intermediate step failes, we must be able to go back where we were + prev_sel_file = self.selected_file + # handling of entire paths with multiple directories/elements if '/' in name: - prev_sel_file = self.selected_file pathlist = name.split('/') # treat /DF.GSM/foo like MF/DF.GSM/foo if pathlist[0] == '': @@ -327,41 +365,29 @@ try: for p in pathlist: self.select(p, cmd_app) - return + return self.selected_file_fcp except Exception as e: - # if any intermediate step fails, go back to where we were self.select_file(prev_sel_file, cmd_app) raise e + # we are now in the directory where the target file is located + # so we can now refer to the get_selectables() method to get the + # file object and select it using select_file() sels = self.selected_file.get_selectables() if is_hex(name): name = name.lower() - self._select_pre(cmd_app) + try: + if name in sels: + f = sels[name] + self.select_file(sels[name], cmd_app) + else: + self.probe_file(name, cmd_app) + except Exception as e: + self.select_file(prev_sel_file, cmd_app) + raise e - if name in sels: - f = sels[name] - try: - if isinstance(f, CardADF): - (data, sw) = self.rs.card.select_adf_by_aid(f.aid, scc=self.scc) - else: - (data, sw) = self.scc.select_file(f.fid) - self.selected_file = f - except SwMatchError as swm: - k = self.interpret_sw(swm.sw_actual) - if not k: - raise(swm) - raise RuntimeError("%s: %s - %s" % (swm.sw_actual, k[0], k[1])) - select_resp = f.decode_select_response(data) - else: - (select_resp, data) = self.probe_file(name, cmd_app) - - # store the raw + decoded FCP for later reference - self.selected_file_fcp_hex = data - self.selected_file_fcp = select_resp - - self._select_post(cmd_app) - return select_resp + return self.selected_file_fcp def status(self): """Request STATUS (current selected file FCP) from card.""" -- To view, visit https://gerrit.osmocom.org/c/pysim/+/34932?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: I9ae213f3b078983f3e6d4c11db38fdbe504c84f2 Gerrit-Change-Number: 34932 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: newchange