<p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c">File src/db_hlr.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/osmo-hlr/+/22311/5/src/db_hlr.c@626">Patch Set #5, Line 626:</a> <code style="font-family:monospace,monospace">}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(add a blank line)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c@638">Patch Set #5, Line 638:</a> <code style="font-family:monospace,monospace">int db_subscrs_get_print(struct vty *vty, struct db_context *dbc, const char *filter_type, const char *filter,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Good solution: in fact this uses a callback function -- which is more elegant than using the vty dir […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c@644">Patch Set #5, Line 644:</a> <code style="font-family:monospace,monospace">subscr</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why do you need this pointer? I think you can use 'sub' directly.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The code I modelled this on had a pointer to a hlr_subscriber struct passed in, so I was mimicking that. <br>I think it might be OK now, but I'm never 100% sure.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/db_hlr.c@689">Patch Set #5, Line 689:</a> <code style="font-family:monospace,monospace">         subscr->id = sqlite3_column_int64(stmt, 0);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(along with fixeria's comment: here you could do […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does it zero init the entire struct? I sort of depend on that later</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/hlr_vty_subscr.c">File src/hlr_vty_subscr.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/osmo-hlr/+/22311/5/src/hlr_vty_subscr.c@229">Patch Set #5, Line 229:</a> <code style="font-family:monospace,monospace"> rc = db_subscrs_get_print(vty, g_hlr->dbc, filter_type, filter, subscr_dump_summary_vty, &count, &err);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">to call the pre-existing subscr_dump_summary_vty(): to match the 'void *data' as I described before  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No objection, but can I ask why, just to understand?<br>Is it better to cast data to a vty struct this way rather than the declaration in:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">subscr_dump_summary_vty(struct hlr_subscriber *subscr, void *data)<br>{<br>     struct vty *vty = data;<br>       .....<br>}</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/src/hlr_vty_subscr.c@230">Patch Set #5, Line 230:</a> <code style="font-family:monospace,monospace">   if (count > 40) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">hm, this code dup is not so nice. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yep... I was doubting it also, but given that the patch set was unlikely to attract +2 on first run, I left it for comments. 40 was just an arbitrary number - of course we have no idea what size the terminal is.<br>All the same, if somebody modifies the header without modifying the footer, then they get a slap on the wrist...?</p><p style="white-space: pre-wrap; word-wrap: break-word;">I could just drop the footer, it's not like it's not obvious which column is which. <br>I don't like reprinting the header, in can get in the way if one did want to copy rows from the terminal or screen scrape the vty (not suggesting I wanty to do this)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311/5/tests/test_nodes.vty">File tests/test_nodes.vty:</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/osmo-hlr/+/22311/5/tests/test_nodes.vty@17">Patch Set #5, Line 17:</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;"> show subscribers filter (imsi|msisdn) FILTER<br>  show subscribers filter (cs|ps) (on|off)<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would actually go for just "show subscribers (imsi|msisdn) FILTER" without the "filter' part. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311">change 22311</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/osmo-hlr/+/22311"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-hlr </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I7f0573381a6d0d13841ac6d42d50f0e8389decf4 </div>
<div style="display:none"> Gerrit-Change-Number: 22311 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 19 Jan 2021 18:20:17 +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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>