<p>Patch set 2:<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-hnodeb/+/26503">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/include/osmocom/hnodeb/hnb_prim.h">File include/osmocom/hnodeb/hnb_prim.h:</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-hnodeb/+/26503/2/include/osmocom/hnodeb/hnb_prim.h@243">Patch Set #2, Line 243:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">uint8_t remote_gtpu_address_type;<br> union u_addr remote_gtpu_addr;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">same comment about address like in the RTP patch</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/include/osmocom/hnodeb/hnb_prim.h@286">Patch Set #2, Line 286:</a> <code style="font-family:monospace,monospace"> un</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same comment about moving context_id up one layer as in RTP patch</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c">File src/osmo-hnodeb/gtp.c:</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-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@63">Patch Set #2, Line 63:</a> <code style="font-family:monospace,monospace"> LOG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">not sure we want that level of verbosity in a user plane code path? you will get a Tx... line at the same log level with the same information 7 lines below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@70">Patch Set #2, Line 70:</a> <code style="font-family:monospace,monospace">req</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">comment says request, next line says indication. Further comment below again says req.</p><p style="white-space: pre-wrap; word-wrap: break-word;">One ideq aould be to move the logging of the message type into a log macro, which simply uses a value_string to print the message type? Not runtime efficient due to the value_string lookup, but neither DEBUG nor ERROR should happen _that_ often in a production deployment?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@126">Patch Set #2, Line 126:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">}<br><br> osmo_fd_setup(&hnb->gtp.fd0, gsn->fd0, OSMO_FD_READ, hnb_gtp_fd_cb, hnb, 0);<br> if ((rc = osmo_fd_register(&hnb->gtp.fd0)) < 0)<br> goto free_ret;<br><br> osmo_fd_setup(&hnb->gtp.fd1c, gsn->fd1c, OSMO_FD_READ, hnb_gtp_fd_cb, hnb, 1);<br> if ((rc = osmo_fd_register(&hnb->gtp.fd1c)) < 0)<br> goto free_ret;<br><br> osmo_fd_setup(&hnb->gtp.fd1u, gsn->fd1u, OSMO_FD_READ, hnb_gtp_fd_cb, hnb, 2);<br> if ((rc = osmo_fd_register(&hnb->gtp.fd1u)) < 0)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The Iuh interface only uses GTP-U v1, neither GTPv0 and no GTPv1-C. So why are we creating three sockets and instantianting an entire GSN here?</p><p style="white-space: pre-wrap; word-wrap: break-word;">The HNB is no GSN. GSNs are SGSN and GGSN.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't see the advantage of using libgtp here. What is the rationale of using it? AFAICT all we need for GTPv1U is to push one static, fixed size GTP header in front of every TX packet, and parse/remove the same fixed-size header in the RX direction.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm afraid by uising a complete "GSN" implementation we get a lot of stuff that we don't want here, and which might create trouble. But maybe you did a thorough study of libgtp and decided it is a better option?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@180">Patch Set #2, Line 180:</a> <code style="font-family:monospace,monospace"> LOG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">where deos this constraint come from? libgtp?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503">change 26503</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-hnodeb/+/26503"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-hnodeb </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I5a6f5dfc4e508c92adb35210b4dc576d64353366 </div>
<div style="display:none"> Gerrit-Change-Number: 26503 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 12 Dec 2021 08:00:24 +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>