<p>Patch set 5:<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/12763">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c">File src/osysmon_openvpn.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/12763/5/src/osysmon_openvpn.c@68">Patch Set #5, Line 68:</a> <code style="font-family:monospace,monospace">                  get_authority(NULL, vpn->cfg), msgb_length(msg), 128);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">get_authority memleak,</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@74">Patch Set #5, Line 74:</a> <code style="font-family:monospace,monospace">                fprintf(stderr, "[%s] received message is empty, ignoring...\n", get_authority(NULL, vpn->cfg));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">get_authority memleak.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@79">Patch Set #5, Line 79:</a> <code style="font-family:monospace,monospace">    tmp[sizeof(tmp) - 1] = '\0';</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if that's a string, then you can simply use osmo_strlcpy afaik.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@82">Patch Set #5, Line 82:</a> <code style="font-family:monospace,monospace">                if (tok)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">add missing {}, otherwise it's confusing. Only use conditionals without bracket for one-line sections.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@83">Patch Set #5, Line 83:</a> <code style="font-family:monospace,monospace">                     switch (i++) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if I understand correctly, first time switch is checked against i=0 (because it's incremented afterwards), and I see no 0 case here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@127">Patch Set #5, Line 127:</a> <code style="font-family:monospace,monospace">       if (msg == NULL) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(!msg)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@134">Patch Set #5, Line 134:</a> <code style="font-family:monospace,monospace">             fprintf(stderr, "[%s] unable to receive message in callback\n", get_authority(NULL, vpn->cfg));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">mem leak, get_authority allocates stuff.</p><p style="white-space: pre-wrap; word-wrap: break-word;">also, msg is leaked.</p><p style="white-space: pre-wrap; word-wrap: break-word;">better use goto.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@149">Patch Set #5, Line 149:</a> <code style="font-family:monospace,monospace">               return true;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">that's weird from API point of view. You usually expect a "create" API to allocate and return newly-allocated struct. Otherwise you never know if a new struct was allocated (which needs to be freed at some point) or an already previously struct was returned (and then avoid double freeing).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Calling it "find_or_create" would be more clear.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@159">Patch Set #5, Line 159:</a> <code style="font-family:monospace,monospace">            talloc_free(vpn);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Instead of talloc_free + return false in lots of places, use a goto to end of function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@178">Patch Set #5, Line 178:</a> <code style="font-family:monospace,monospace">     osmo_stream_cli_set_reconnect_timeout(vpn->mgmt, 60); /* FIXME: this seems to be ignored by libosmo-netif */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">was this already fixed? otherwise please open a ticket and put the ticket number in the commit for later reference.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@263">Patch Set #5, Line 263:</a> <code style="font-family:monospace,monospace">           value_node_add(vn_host, "remote", remote);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">possible memleak of remote later on, to be checked.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@267">Patch Set #5, Line 267:</a> <code style="font-family:monospace,monospace">              msgb_printf(msg, "state\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure what is this for.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12763/5/src/osysmon_openvpn.c@287">Patch Set #5, Line 287:</a> <code style="font-family:monospace,monospace">     if (llist_count(&g_oss->openvpn_clients)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need to count, just check if the list is not empty.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12763">change 12763</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/12763"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-sysmon </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I4493e19b9a09dcebd289457eacd1719f7f8cc31c </div>
<div style="display:none"> Gerrit-Change-Number: 12763 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 01 Feb 2019 16:34:56 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>