This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgPatch Set 3: Code-Review-1 (27 comments) https://gerrit.osmocom.org/#/c/2884/3/src/osmo_gsm_tester/esme.py File src/osmo_gsm_tester/esme.py: Line 37: self.system_id = 'esme-id' each new Esme class gets an identical system_id, which gets put in a config in a for loop, i.e. it's easy to mistakenly create multiple esmes with the same ID? Line 41: self.binded = False "bound" ... 'binded' is not a word Line 42: self.listen_flag = False "flag" is ambiguous. rather name it "is_listening" or "listening" Line 54: self.system_id = name It's easy to create an Esme with system_id == None? then the config file would say "None" as id? Line 57: self.password = password does self.password == None produce a valid config? Line 60: config = { 'system_id': self.system_id, 'password': self.password } (I kinda like using dict(name=val, name2=val2), because it has less quoting, but it's up to you) Line 64: self.client.poll() why handler? not just "poll()"? Line 71: if self.listen_flag: early exit Line 90: self.log('Connected and binded successfully. Start listening') "bound"; "Starting to listen" Line 100: self.connected = False I'm wondering, will there ever be a valid state where we're not all of the three, connected, bound and listening? Does it make sense to keep only one flag for all three? https://gerrit.osmocom.org/#/c/2884/3/src/osmo_gsm_tester/osmo_msc.py File src/osmo_gsm_tester/osmo_msc.py: Line 25: class OsmoMsc(log.Origin, smsc.Smsc): I would prefer using composition instead of multiple inheritance: OsmoMsc.smsc = Smsc(...) Line 36: smsc.Smsc.__init__(self, (ip_address.get('addr'), 2775)) ...because multiple inheritance is a bit ugly, in pretty much every language. https://gerrit.osmocom.org/#/c/2884/3/src/osmo_gsm_tester/sms.py File src/osmo_gsm_tester/sms.py: Line 24: def __init__(self, src_msisdn=None, dst_msisdn=None, *tokens): any particular reason for this rename? https://gerrit.osmocom.org/#/c/2884/3/src/osmo_gsm_tester/smsc.py File src/osmo_gsm_tester/smsc.py: Line 41: def esme_add(self, esme): validate against dup ids or empty passwords? Line 46: return self.addr_port can't callers just use smsc.addr_port directly? https://gerrit.osmocom.org/#/c/2884/2/src/osmo_gsm_tester/suite.py File src/osmo_gsm_tester/suite.py: Line 351: def msisdn(self): > This change was needed, otherwise I get an error with something like "suite looks like a bugfix indeed. So far we used only subscriber_add(), never this function yet. https://gerrit.osmocom.org/#/c/2884/3/src/osmo_gsm_tester/templates/osmo-msc.cfg.tmpl File src/osmo_gsm_tester/templates/osmo-msc.cfg.tmpl: Line 27: policy ${msc.smsc.policy} as with mgcpgw, it would make sense to just use 'smsc.policy' as key, dropping the 'msc' part. https://gerrit.osmocom.org/#/c/2884/3/src/osmo_gsm_tester/templates/osmo-nitb.cfg.tmpl File src/osmo_gsm_tester/templates/osmo-nitb.cfg.tmpl: Line 80: policy ${nitb.smsc.policy} drop 'nitb.' from key https://gerrit.osmocom.org/#/c/2884/3/suites/aoip_smpp/esme_connect_policy_acceptall.py File suites/aoip_smpp/esme_connect_policy_acceptall.py: Line 18: # here we deliberately ommit calling msc.esme_add() to avoid having it included in the smsc config. "omit" Line 25: # Connect should work even if we didn't previously configure the esme in the smsc. "Due to accept-all policy," https://gerrit.osmocom.org/#/c/2884/3/suites/aoip_smpp/esme_connect_policy_closed.py File suites/aoip_smpp/esme_connect_policy_closed.py: Line 31: # Connect should work as we configured msc properly at esme_add() time. the log message already says it, drop the comment? (also below) Line 39: new_password = 'barfoo' if correct_password == 'foobar' else 'foobar' really? lol why not just new_password = 'garble'? Line 47: new_system_id = 'barfoo' if correct_system_id == 'foobar' else 'foobar' 'garble'? https://gerrit.osmocom.org/#/c/2884/3/suites/aoip_smpp/esme_ms_sms.py File suites/aoip_smpp/esme_ms_sms.py: Line 52: # esme.run_method_expect_failure(SMPP_ESME_RINVDSTADR, esme.sms_send, msg) (I think we accept SMS for any MSISDN? As in, deliver it once the subscriber becomes available, without checking that a destination is valid in advance.) https://gerrit.osmocom.org/#/c/2884/3/suites/smpp/esme_connect_policy_acceptall.py File suites/smpp/esme_connect_policy_acceptall.py: Line 12: # here we deliberately ommit calling nitb.esme_add() to avoid having it included in the smsc config. "omit" Line 15: # Connect should work even if we didn't previously configure the esme in the smsc. "Due to accept-all policy..." https://gerrit.osmocom.org/#/c/2884/3/suites/smpp/esme_connect_policy_closed.py File suites/smpp/esme_connect_policy_closed.py: Line 22: # Connect should work as we configured nitb properly at esme_add() time. drop comments -- To view, visit https://gerrit.osmocom.org/2884 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I14ca3cb009d6d646a449ca99b0200da12085c0da Gerrit-PatchSet: 3 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de> Gerrit-HasComments: Yes