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 7: Code-Review-1
(21 comments)
Despite the numerous comments and the -1 I really like this. Let's resolve the async question, that's the important part; and if you like resolve some of the cosmetics, most of which are no show-stoppers.
https://gerrit.osmocom.org/#/c/2779/7//COMMIT_MSG
Commit Message:
Line 10:
you could elaborate a bit on what is going on in this commit. Some say to have one comment for each patch chunk, which may be a bit overboard, but it would be good if the commit log explanations roughly match the complexity of the patch.
https://gerrit.osmocom.org/#/c/2779/7/src/osmo_gsm_tester/ofono_client.py
File src/osmo_gsm_tester/ofono_client.py:
Line 90: etype, e = sys.exc_info()[:2]
are you using etype? why not
except Exception as e:
Line 108: *proxymethod_args,
indent?
Line 113: #def dbus_async_call(proxymethod, instance, *args, **kwargs):
cruft?
Line 128: GLib.Variant(proxymethod._sinargs, proxymethod_args), GLib.VariantType.new(proxymethod._soutargs),
(cosmetic: each GLib.* call on its own line?)
Line 402: nr = self.dbus.interface(I_NETREG)
(cosmetic: I read 'nr' as 'number'. netreg?)
Line 410: def scan_error_cb(self, obj, e, user_data):
interesting question here: is scan_error_cb() called asyncly? Should it rather have a defer() shim? so that we are doing the actual handling in our main thread and are sure to catch exceptions etc
Line 413: def scan_result_cb(self, obj, result, user_data):
same async question
Line 418: if not self.is_connected():
is_connected() again checks "self.mcc_mnc is None" and so forth. Does it make sense to do this above for both code paths:
if self.is_connected():
# already on desired operator
return
And I think you said it before ... but from reading the code it is unclear: there was an is_connected() check when starting to scan, here is another one. I would welcome a code comment stating why it has to be checked again.
Line 423: self.log('Already registered with the network')
Osmo style: always early-exit instead of if-cascade:
if not_needed_condition:
return
otherwise_needed_action()
Line 426: myop = None
(cosmetic: 'matching_op_path')
Line 433: self.log('Already registered with the network')
seems to me to duplicate the is_connected() semantics ... follows above comment.
Line 434: # return and not try to re-connect, otherwise dbus method returns a fail
"otherwise" meaning what? not return?? :)
It seems obvious that we don't try to re-connect when already registered with desired operator, only this comment confuses me, is it trying to say more than that?
Line 447: 'Power the modem and put it online'
', power cycle it if it was already on'
Line 448: self.dbg('Powering on')
(this dbg is a bit confusing. If it is already powered, we will in fact power it off, not on. Powering on will happen later.)
Line 450: self.dbg('is powered')
(log says "Powering on", "is powered" -- but instead the facts are: modem was powered before, now powering off; this here could say "Power cycling")
Line 453: event_loop.wait(self, lambda: not self.dbus.has_interface(I_NETREG, I_SMS), timeout=10)
and here maybe
else:
self.dbg('Powering on')
Line 459: 'Connect to MCC+MNC from NITB config'
rather don't mention the NITB config at all.
Line 460: if mcc_mnc is not None and (len(mcc_mnc) != 2 or None in mcc_mnc):
(personal preference: braces around each sub-condition)
Line 464: self.log('connect to', self.mcc_mnc if self.mcc_mnc else 'default network')
this log could go in the 'else:' below? In the sense of unbloating the log: instead of logging two lines, log one per code path (each with all the info)
https://gerrit.osmocom.org/#/c/2779/7/src/osmo_gsm_tester/osmo_msc.py
File src/osmo_gsm_tester/osmo_msc.py:
Line 40: self.config = None
for things that are None I generally put them above in the, what is it called, "static" section, like hlr = None. (Got to be careful there with things like lists, dicts or objects which could suddenly be shared between all instances of the class, but with None there is no such problem)
--
To view, visit https://gerrit.osmocom.org/2779
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d9eb47eac1044550d3885adb55105c304b0c15c
Gerrit-PatchSet: 7
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