<p style="white-space: pre-wrap; word-wrap: break-word;">meta: Thanks for preparing so far! I can also take this patch over from you, if you like.</p><p style="white-space: pre-wrap; word-wrap: break-word;">There are a lot of copy-paste errors in the vty code, needs a vty transcript test to uncover all.</p><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/12446">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/12446/2/include/osmocom/msc/neighbor_ident.h">File include/osmocom/msc/neighbor_ident.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/12446/2/include/osmocom/msc/neighbor_ident.h@26">Patch Set #2, Line 26:</a> <code style="font-family:monospace,monospace"> } a;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">a as in "A-interface"??</p><p style="white-space: pre-wrap; word-wrap: break-word;">I learned recently that one can actually have unnamed unions...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c">File src/libmsc/neighbor_ident.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/12446/2/src/libmsc/neighbor_ident.c@35">Patch Set #2, Line 35:</a> <code style="font-family:monospace,monospace">#define NEIGHBOR_IDENT_ADDR_STRING_MAX 64</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">hmm, if we have a maximum length for this, then rather use a fixed-size char[] array in neighbor_ident_addr.<br>It also clarifies ownership and memory management.<br>I was going to approve talloc strings there to provide arbitrary length names, but if we limit it then let's array.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@39">Patch Set #2, Line 39:</a> <code style="font-family:monospace,monospace"> static char buf[NEIGHBOR_IDENT_ADDR_STRING_MAX + 4];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">+4 for "BSC ". +1 for '\0'?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@106">Patch Set #2, Line 106:</a> <code style="font-family:monospace,monospace"> * replace an existing entry, first call neighbor_ident_del(nil, cell_id).</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">neighbor_ident_del(nil, addr)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@109">Patch Set #2, Line 109:</a> <code style="font-family:monospace,monospace"> * return -ENOMEM when the list is not initialized, -ERANGE when the BSIC value is too large. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">BSIC??</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident.c@159">Patch Set #2, Line 159:</a> <code style="font-family:monospace,monospace"> struct gsm0808_cell_id *cell_id)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It should be possible to detect multiple matches.<br>Suggest arg match_idx as in gsm_bts_by_cell_id() in osmo-bsc/src/osmo-bsc/gsm_data.c.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(I now think we should also add an exact_match argument as well; that would also have to be added to gsm0808_cell_id_matches_list())</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c">File src/libmsc/neighbor_ident_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/12446/2/src/libmsc/neighbor_ident_vty.c@39">Patch Set #2, Line 39:</a> <code style="font-family:monospace,monospace">#define NEIGHBOR_DOC "Manage neighbor BSS cells\n"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"Manage local- and remote-MSC BSS cells for inter-BSC and inter-MSC handover\n"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@134">Patch Set #2, Line 134:</a> <code style="font-family:monospace,monospace">" "</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no " " in DOC part, the docs are separated by '\n'; same N times below</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@140">Patch Set #2, Line 140:</a> <code style="font-family:monospace,monospace"> vty_out(vty, "Could not parse point code '%s'%s", argv[0], VTY_NEWLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">wrong argv[idx] for point code arg.<br>preferably use a local const char *pc_str = &argv[1];<br>same N times below</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@146">Patch Set #2, Line 146:</a> <code style="font-family:monospace,monospace"> return add_neighbor(vty, &addr, neighbor_ident_vty_parse_lac(vty, argv + 1));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">+ 1 seems wrong; same N times below</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@147">Patch Set #2, Line 147:</a> <code style="font-family:monospace,monospace">}</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please add a .vty transcript test to verify that all of these commands work as expected.<br>See for example osmo-bsc/tests/neighbor_ident.vty</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@261">Patch Set #2, Line 261:</a> <code style="font-family:monospace,monospace"> SHOW_STR "Delete a neighbor MSC\n" "MSC ipa-nam\n"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">SHOW_STR???</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12446/2/src/libmsc/neighbor_ident_vty.c@373">Patch Set #2, Line 373:</a> <code style="font-family:monospace,monospace"> SHOW_STR "Display information about a neighbor MSC\n" "MSC ipa-name\n"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No, you can't overload the same command token with two differing doc strings (above, the same one said "BSC").<br>Only one of them will be visible on the VTY interface.<br>Instead you need to "Display information about a neighbor MSC or BSS\n" and make the distinction only with the actually differing cmd token ('msc-*' or 'bsc-*').<br>Define one single NEIGHBOR_SHOW_DOC and use in both instances.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12446">change 12446</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/12446"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ia0dd08b087bfd4aa22e234917669d003150a4cd4 </div>
<div style="display:none"> Gerrit-Change-Number: 12446 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 14 Jan 2019 20:24:33 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>