<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274/1/sysmocom/suites/4g/iperf3_ul.py">File sysmocom/suites/4g/iperf3_ul.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274/1/sysmocom/suites/4g/iperf3_ul.py@19">Patch Set #1, Line 19:</a> <code style="font-family:monospace,monospace">ue = []</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this naming is a bit confusing, it seems as it should contain only 1 ue. ue_li?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274/1/sysmocom/suites/4g/iperf3_ul.py@31">Patch Set #1, Line 31:</a> <code style="font-family:monospace,monospace">  iperf3srv.append(tenv.iperf3srv({'addr': epc.tun_addr()}, n))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">here you can use the .set_port() API ;)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274/1/sysmocom/suites/4g/iperf3_ul.py@58">Patch Set #1, Line 58:</a> <code style="font-family:monospace,monospace">proc = []</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Naming: Same here, proc is usually used for a single proc object, not a list.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274/1/sysmocom/suites/4g/iperf3_ul.py@67">Patch Set #1, Line 67:</a> <code style="font-family:monospace,monospace">  wait(ue[n].is_registered)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You can speed up the test by moving this wait to a follow-up loop, so that you start all UE in parallel in the background and then wait for all of them to be registered.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274/1/sysmocom/suites/4g/iperf3_ul.py@74">Patch Set #1, Line 74:</a> <code style="font-family:monospace,monospace">  threads.append(threading.Thread(target=run_iperf, args=(proc[n],)))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You don't really need threads here, and better not use them unless really necessarily, since you may end up with dangling threads if something fails (exception thrown by test or osmo-gsm-tester core).<br>See how to do it properly here:<br>osmo-gsm-tester/sysmocom/suites/gprs/iperf3m4.py<br>osmo-gsm-tester/sysmocom/suites/gprs/lib/testlib.py</p><p style="white-space: pre-wrap; word-wrap: break-word;">In summary:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">    try:<br>        for proc in procs:<br>            proc.launch()<br>        if ready_cb:<br>            ready_cb(ms_li)<br>        for proc in procs:<br>            proc.wait()<br>    except Exception as e:<br>        for proc in procs:<br>            try:<br>                proc.terminate()<br>            except Exception:<br>                print("Exception while terminating process %r" % repr(process))<br>        raise e</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274">change 24274</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/24274"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-gsm-tester </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I4b006df04bd1af6c117bcb25e6a6b1609ac732fb </div>
<div style="display:none"> Gerrit-Change-Number: 24274 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Alejandro Leal <alejandro.leal@srs.io> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 20 May 2021 12:42:06 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>