<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -2</span></p><p><a href="https://gerrit.osmocom.org/12814">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12814/2//COMMIT_MSG">Commit Message:</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/12814/2//COMMIT_MSG@13">Patch Set #2, Line 13:</a> <code style="font-family:monospace,monospace">CTRL command access mode indicator</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It looks like you're submitting two squashed changes in a single one.<br>Why not to submit both separately?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12814/2/include/osmocom/ctrl/control_cmd.h">File include/osmocom/ctrl/control_cmd.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/12814/2/include/osmocom/ctrl/control_cmd.h@107">Patch Set #2, Line 107:</a> <code style="font-family:monospace,monospace">enum ctrl_cmd_mode mode;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You're breaking ABI here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/include/osmocom/ctrl/control_cmd.h@152">Patch Set #2, Line 152:</a> <code style="font-family:monospace,monospace">CTRL_CMD_DEFINE_STRUCT</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since this is a public header, we cannot modify existing symbols nor definitions.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/include/osmocom/ctrl/control_cmd.h@216">Patch Set #2, Line 216:</a> <code style="font-family:monospace,monospace">CTRL_MODE_RW</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same comments apply here and below...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_cmd.c">File src/ctrl/control_cmd.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/12814/2/src/ctrl/control_cmd.c@61">Patch Set #2, Line 61:</a> <code style="font-family:monospace,monospace">\a handle_ctrl_cmd</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">AFAIR, Neels recently figured out that manual referencing (\ref or \a) is not needed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_cmd.c@74">Patch Set #2, Line 74:</a> <code style="font-family:monospace,monospace">cmds_vec = vector_lookup_ensure(ctrl_node_vec, node</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">cosmetic: you could reduce nesting:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  cmds_vec = vector_lookup_ensure(ctrl_node_vec, node);<br>  if (!cmds_vec)<br>    return 0; // or -EFOOBAR?</pre></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c">File src/ctrl/control_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/12814/2/src/ctrl/control_vty.c@83">Patch Set #2, Line 83:</a> <code style="font-family:monospace,monospace">i=0</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing spaces.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@89">Patch Set #2, Line 89:</a> <code style="font-family:monospace,monospace"> switch(cmd_el->mode){</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing space, should be 'switch ('.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@90">Patch Set #2, Line 90:</a> <code style="font-family:monospace,monospace">  </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Usually, we don't shift 'cases' from the 'switch' statement.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@100">Patch Set #2, Line 100:</a> <code style="font-family:monospace,monospace">""</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"(unknown)"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@104">Patch Set #2, Line 104:</a> <code style="font-family:monospace,monospace">talloc_free</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This allocation is only used at the beginning of the function, so it should be freed there.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@109">Patch Set #2, Line 109:</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;">DEFUN(show_ctrl_asciidoc_table,<br>              show_ctrl_asciidoc_table_cmd,<br>"show asciidoc ctrl",<br>SHOW_STR "Asciidoc generation\n" "Generate table of all registered ctrl configurations\n")<br>{<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This coding style itself deserves CR-2, sorry.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12814">change 12814</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/12814"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I16f59bfca72a7dd9c268bc64499b26d82a2115d2 </div>
<div style="display:none"> Gerrit-Change-Number: 12814 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Omar Ramadan <omar.ramadan93@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 04 Feb 2019 06:14:58 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>