<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/libosmocore/+/21243">View Change</a></p><p>19 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c">File src/gb/frame_relay.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/libosmocore/+/21243/1/src/gb/frame_relay.c@106">Patch Set #1, Line 106:</a> <code style="font-family:monospace,monospace">/* RX Message: 14 [ 00 01 03 08 00 75 95 01 01 00 03 02 01 00 ] */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this can be dropped right?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@119">Patch Set #1, Line 119:</a> <code style="font-family:monospace,monospace">// .default_val = 10,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@222">Patch Set #1, Line 222:</a> <code style="font-family:monospace,monospace"> ie[2] |= 0x02;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what are these 0x02 0x04 0x08? Please add defines for those.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@276">Patch Set #1, Line 276:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">line can be dropped.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@310">Patch Set #1, Line 310:</a> <code style="font-family:monospace,monospace"> LOGPFRL(link, LOGL_ERROR, "STATUS-ENQ aren't support for role user\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"are not supported"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@343">Patch Set #1, Line 343:</a> <code style="font-family:monospace,monospace"> /* TODO: implement FRNET free */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">sounds like leaking memory?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@366">Patch Set #1, Line 366:</a> <code style="font-family:monospace,monospace"> link->succeed = link->succeed << 1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">link->succeed <<= 1;</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@370">Patch Set #1, Line 370:</a> <code style="font-family:monospace,monospace"> /* count the bits */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It would be great to explain a bit better what exactly are you doing here or provide some spec reference.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@461">Patch Set #1, Line 461:</a> <code style="font-family:monospace,monospace"> LOGPFRL(link, LOGL_ERROR, "Could not create DLC %d\n", dlci);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">return, or directly OSMO_ASERT.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@468">Patch Set #1, Line 468:</a> <code style="font-family:monospace,monospace"> dlc->add = pvc->new;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">not sure if this "new" here will cause problems when compiling with c++.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@657">Patch Set #1, Line 657:</a> <code style="font-family:monospace,monospace"> tlv_parse2(tp, MAX_SUPPORTED_PVC + 1, &q933_att_tlvdef, msgb_l3(msg) + sizeof(*qh), msgb_l3len(msg) - sizeof(*qh), 0, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">check return code.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/frame_relay.c@889">Patch Set #1, Line 889:</a> <code style="font-family:monospace,monospace">/* TODO: osmo_fr_dlc_alloc with deregistering it from the link in fr-net */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">you probably mean _free here?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2.c">File src/gb/gprs_ns2.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/libosmocore/+/21243/1/src/gb/gprs_ns2.c@262">Patch Set #1, Line 262:</a> <code style="font-family:monospace,monospace"> snprintf(buf, buf_len, "fr)netif: %s dlci: %s", "hdlcX", "unsupported");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this needs work to print correct variables right?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c">File src/gb/gprs_ns2_fr.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/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c@86">Patch Set #1, Line 86:</a> <code style="font-family:monospace,monospace">struct iphdr</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Don't we have this defined somewhere else? We should move this to some osmocom compat header IMHO, similar to what we do with timespecs, etc.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c@119">Patch Set #1, Line 119:</a> <code style="font-family:monospace,monospace"> char netif[IF_NAMESIZE + 1];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">IFNAMSIZ. That's the maximum size including the null char.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c@263">Patch Set #1, Line 263:</a> <code style="font-family:monospace,monospace"> /* FIXME */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this should be added now right?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_fr.c@301">Patch Set #1, Line 301:</a> <code style="font-family:monospace,monospace"> /* FIXME half writes */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">IIUC this should put stuff into a wqueue?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_vty.c">File src/gb/gprs_ns2_vty.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/libosmocore/+/21243/1/src/gb/gprs_ns2_vty.c@86">Patch Set #1, Line 86:</a> <code style="font-family:monospace,monospace"> char netif[IF_NAMESIZE + 1];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove +1</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243/1/src/gb/gprs_ns2_vty.c@484">Patch Set #1, Line 484:</a> <code style="font-family:monospace,monospace"> uint16_t dlci = atoi(argv[2]);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This looks wrong, I only see 2 arguments in this vty cmd.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/21243">change 21243</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/libosmocore/+/21243"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id3b49f93d33c271f77cd9c9db03cde6b727a4d30 </div>
<div style="display:none"> Gerrit-Change-Number: 21243 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </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, 19 Nov 2020 10:05:08 +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>