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/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302 )
Change subject: Introduce Android UEs as new modems
......................................................................
Patch Set 3: Code-Review-1
(13 comments)
This is starting to look good, there's only a few things here and there which needs polishing.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py
File src/osmo_gsm_tester/obj/android_apn.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@30
PS3, Line 30: schema.register_resource_schema('apn', resource_schema)
This is probably not correct, as this should rpobably be under "modem" object or alike. So modem will probably end up adding this by using AndroidApn.schema() on its on_register_schemas()
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@71
PS3, Line 71: def from_conf(cls, conf):
This one is PUBLIC INTERNAL API
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@75
PS3, Line 75: @classmethod
This one is PUBLIC INTERNAL API
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_apn.py@87
PS3, Line 87: proc = self.run_androidue_cmd("set-apn", [qry_carrier_cmd])
"get-carrier-id" instead of "set-apn"? or is set-apn the binary? Not sure now if this is a description string or the binary name.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py
File src/osmo_gsm_tester/obj/android_host.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@31
PS3, Line 31: def __init__(self, name, testenv, conf):
what's the point in passing these params to constructor if you don't use them? You can remove them
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@42
PS3, Line 42: log.dbg("Deleted the following files: %s" % clear_run_dir_proc.get_stdout())
You should better inherit from log.Origin in AndroidHost and use self.log. See how other classes do it.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@50
PS3, Line 50: self.dbg("Deleted the following files: %s" % clear_diag_logs_proc.get_stdout())
see, you already expect to be inheriting from log.Origin, but you are not ;)
This probably works because subclasses are inheriting from log.Origin. You should be inheriting here instead, and call this constructor from child classes instead.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/android_host.py@72
PS3, Line 72: if self._run_node.is_local():
I would welcome some description here too, specially what's the main difference with the method above and the rationale for having both.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/iperf3.py
File src/osmo_gsm_tester/obj/iperf3.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/iperf3.py@297
PS3, Line 297: elif self._run_node.ssh_port():
So the main difference between this branch and the one below is that you need to force "root" user?
I'm not liking checking here based on whether ssh port is set or not. We should flag this somehow in the class implementing when passing its run_node by means of this class set_run_node().
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/iperf3.py@315
PS3, Line 315: self.process = process.Process(self.name(), self.run_dir, popen_args, env={})
Please add a new AdbProcess sublcass similar to NetNSProcess to hide this adb command stuff.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/ms_android.py
File src/osmo_gsm_tester/obj/ms_android.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/ms_android.py@43
PS3, Line 43: resource_schema['apn.%s' % key] = val
what I wrote in the first file of this patch: This is done here, so no need to have it in register_schemas() on the AndroidApn file.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/ms_android.py@257
PS3, Line 257: ip_prefix = "172.16.0"
This API is probably PROTECTED?
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/qc_diag.py
File src/osmo_gsm_tester/obj/qc_diag.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/3/src/osmo_gsm_tester/obj/qc_diag.py@81
PS3, Line 81: while timer > 0:
I think in general you should be using event_loop.py's wait() method for this kind of sleeping loops.
See for instance:
"""
MainLoop.wait(self.dbus.has_interface, *req_ifaces, timeout=10)
"""
--
To view, visit https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Change-Id: I79a5d803e869a868d4dac5e0d4c2feb38038dc5c
Gerrit-Change-Number: 21302
Gerrit-PatchSet: 3
Gerrit-Owner: ninjab3s <nils.fuerste at softwareradiosystems.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 30 Nov 2020 17:08:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201130/2e6fd588/attachment.htm>