Patch Set 2: (9 comments)
https://gerrit.osmocom.org/#/c/2541/2/src/osmo_gsm_tester/netlogger.py File src/osmo_gsm_tester/netlogger.py: Line 27: class NetLogger(log.Origin): don't really like the name, sounds like the gsm tester would write its own log files to a remote host or something -- maybe PcapRecorder? Line 32: self.host = host are user and host actually used? Line 40: self.log('Starting logger', self.run_dir, self.gen_filter()) same. 'Recording pcap'? Line 42: # TODO: if host not None, launch it remotely I hope we won't need that :) Line 52: def set_addr(self, addr): pass this to constructor instead? Line 55: def set_iface(self, iface): pass this to constructor instead? https://gerrit.osmocom.org/#/c/2541/2/src/osmo_gsm_tester/osmo_nitb.py File src/osmo_gsm_tester/osmo_nitb.py: Line 54: nl = netlogger.NetLogger(self.suite_run, self.run_dir.new_dir('pcap')) the run dir is intended to record stdout and stderr of a running process, as well as possible core dumps. For tcpdump we don't really need that, but whatever. The point being that the path of the resulting pcap being recorded I think should rather be in the suite_run's run_dir directly for easy access by debuggers, and putting it deeper in the tcpdump's run dir could be annoying? What do you think? (Use run_dir.new_file(...) to generate a unique name each time) Line 59: nl.start() maybe do all this in just the constructor call? (i.e. do the iface resolution and the start() implicitly when creating the instance) Store the instance with this nitb for later querying? I see that the process gets stopped from the suite run, but if we at some point call a nitb.stop(), we'd also want to stop the pcap. (admitted, that can be implemented when a stop() is implemented) https://gerrit.osmocom.org/#/c/2541/2/src/osmo_gsm_tester/util.py File src/osmo_gsm_tester/util.py: Line 49: print('Failed to retreive iface:', e) let's drop the print and leave it up to the caller to log an error. -- To view, visit https://gerrit.osmocom.org/2541 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c5d0e2d9857160f905e743517e744f1a06368af Gerrit-PatchSet: 2 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
