<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21243">View Change</a></p><p>16 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/include/osmocom/gprs/frame_relay.h">File include/osmocom/gprs/frame_relay.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/libosmocore/+/21243/1/include/osmocom/gprs/frame_relay.h@1">Patch Set #1, Line 1:</a> <code style="font-family:monospace,monospace">/*! \file frame_relay.h */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">missing copyright / license statement (yes, my fault in the initial code, but I don't want to mess w […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><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@14">Patch Set #1, Line 14:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">missing copyright / license statement (yes, my fault in the initial code, but I don't want to mess w […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">They are the bits we need to use for the "active" "add" and "del" situations. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">line can be dropped.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">can :). I like a newline here because there is an if () without {} there.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"are not supported"</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">works either way, there is no rule that your version is superior or preferred, IMHO. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It would be great to explain a bit better what exactly are you doing here or provide some spec refer […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why? all the other DLCs in the message might just as well have been created? This function is execu […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i'll continue in this case.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">check return code.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">you probably mean _free here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this needs work to print correct variables right?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">We don't really support FreeBSD, Apple or Cygwin platforms, so best to remove it altogether. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">IFNAMSIZ. That's the maximum size including the null char.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">remove +1</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This looks wrong, I only see 2 arguments in this vty cmd.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm still unsure if I want this in the change.</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-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 24 Nov 2020 02:37:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>