<p style="white-space: pre-wrap; word-wrap: break-word;">one important element of reading from a database is to limit the amount of entries, i.e. we wouldn't want to dump the list of all subscribers of an entire continent by accident. So I guess the db_subscr_get() should have a 'limit' argument, or alternatively the get_cb() should be able to terminate reading from the DB by returning false, for example. If the get_cb() also gets an argument indicating the row nr within the filtered result, then the get_cb() could decide to drop out after, say, 1000 entries. Thinking: vty dumping more than (say) 1000 entries should be prevented anyway to make sure osmo-hlr doesn't miss out on handling sockets in time.</p><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/22311">View Change</a></p><p>5 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 style="white-space: pre-wrap; word-wrap: break-word;">(add a blank line)</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 style="white-space: pre-wrap; word-wrap: break-word;">Good solution: in fact this uses a callback function -- which is more elegant than using the vty directly. So this function itself should not need to know what it is passing on to the callback function at all, and it also doesn't need to know whether the subscriber will be printed or what happens to it, i.e. there should be no mention of a 'struct vty' or 'vty' or 'print' here at all. instead, the usual pattern for this situation is to use a 'void *data' arg to be passed to the callback:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> int db_subscrs_get(struct db_context *dbc, [...],<br> void (*get_cb)(struct hlr_subscriber *subscr, void *data),<br> void *data)<br> {<br> ...<br> get_cb(subscr, data);<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Then a get_cb implementation could use the vty like this:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> void subscr_print_vty(struct hlr_subscriber *subscr, void *data)<br> {<br> struct vty *vty = data;<br> vty_out(vty, "...");<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> foo(struct vty *vty)<br> {<br> db_subscrs_get(dbc, [...], subscr_print_vty, vty);<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Like this the db_subscrs_get() is the most generally useful naming and parameter typing, and you happen to use it for printing to vty.</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 style="white-space: pre-wrap; word-wrap: break-word;">(along with fixeria's comment: here you could do</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> sub = (struct hlr_subscriber){<br> .id = sqlite3_column_int64(stmt, 0),<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">which would both zero init and set the id member)</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 style="white-space: pre-wrap; word-wrap: break-word;">to call the pre-existing subscr_dump_summary_vty(): to match the 'void *data' as I described before I'd suggest a simple shim function (which will most likely get optimized away by the compiler) to forward the callback:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> void print_vty_cb(struct hlr_subcsriber *subscr, void *data)<br> {<br> subscr_dump_summary_vty((struct vty*)data, subscr);<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and then</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> db_subscrs_get(g_hlr->dbc, ....., print_vty_cb, vty, &count, &err);</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 style="white-space: pre-wrap; word-wrap: break-word;">hm, this code dup is not so nice. think: if someone modifies the column headers and tests only with < 40 entries, and never notices that the footer names were forgotten to be updated...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Possibly that get_cb() could be passed an index number as well, and the headers could be printed before the first line and again every 40 lines?</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: 5 </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 11:33:35 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>