osmo-gsm-tester[master]: Add support for SMPP testing

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
Thu Jun 15 01:39:12 UTC 2017


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



More information about the gerrit-log mailing list