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.org
Patch 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