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/.
Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Patch Set 3:
(25 comments)
Thanks for the review!
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
Yes, but the ID can be changed using the set_system_id method. I could move the set_msisdn to the constructor and use the unique msisdn there for the initial system-id.
Line 41: self.binded = False
> "bound" ... 'binded' is not a word
Oops true :P
Line 42: self.listen_flag = False
> "flag" is ambiguous. rather name it "is_listening" or "listening"
ACK
Line 54: self.system_id = name
> It's easy to create an Esme with system_id == None? then the config file wo
I think now it doesn't make sense to have it to None, so I'll remove the =None and probably also do some extra checks, like len()>0 and len()<MAX where max I think is defined somewher ein the spec like 14 chars or something similar.
Line 57: self.password = password
> does self.password == None produce a valid config?
Same as above, not sure it makes sense to have None there.
Not sure if it's OK to have an empty password, I'll check that too.
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
I think I found it like this in some places too. I like it, it looks more like json and I usually think as in json when using dictionaries :)
Line 64: self.client.poll()
> why handler? not just "poll()"?
I can change that to poll()
Line 71: if self.listen_flag:
> early exit
ACK
Line 90: self.log('Connected and binded successfully. Start listening')
> "bound"; "Starting to listen"
ACK
Line 100: self.connected = False
> I'm wondering, will there ever be a valid state where we're not all of the
client.connect() or client.bind() can fail, so for sure it can happen :)
For example, during bind SMSC already checks the system-id and password, I am actually testing that (with explicit wrong system-id/password) in some test.
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.s
Ok, I can change that. Is there some specific syntax in python for that? Or I just create a property in the constructor?
self.smsc = smsc.Smsc()
Then I use self.smsc everywhere instead of self?
https://gerrit.osmocom.org/#/c/2884/3/src/osmo_gsm_tester/osmo_nitb.py
File src/osmo_gsm_tester/osmo_nitb.py:
Line 26: class OsmoNitb(log.Origin, smsc.Smsc):
Same here regarding inheritance vs composition
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?
It looks clearer to me, as in any packet you usually use src + dst terminology.
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?
I'll fix the dup ids using the msisdn and the passwd stuff also in the Esme class, no need to do it here.
Line 46: return self.addr_port
> can't callers just use smsc.addr_port directly?
Yes. Initially this was an abstract method to be implemented by subclasses, but finally I just saw I could pass it through the constructor so it got simplified. I can remove the method entirely.
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, dropp
ACK, will do
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
ACK, will do
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"
ACK
Line 25: # Connect should work even if we didn't previously configure the esme in the smsc.
> "Due to accept-all policy,"
ACK, will add it
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)
ACK, will remove this one and add "it should fail" to the prints below + remove the comments.
Line 39: new_password = 'barfoo' if correct_password == 'foobar' else 'foobar'
> really? lol
it's not like adding a lot of code and this way we assert it won't be an issue later if somebody decides to change the default password :)
Let's be paranoic!
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 subscribe
Well when I was manually running nitb with no modem connected to test python-smpplib, I was always receiving this kind of error, so it's probably some part of the config which is different. I need to check which part of the config handles this and tweak it, but it can be done later on, no need to implement the whole SMPP support + all possible tests now. Hopefully somebody will collaborate and add more tests once there are a few of them as an example.
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"
ACK
Line 15: # Connect should work even if we didn't previously configure the esme in the smsc.
> "Due to accept-all policy..."
ACK
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
Will do same as in the other file.
--
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