<p style="white-space: pre-wrap; word-wrap: break-word;">I feel a bit apologetic to -1 again...</p><p>Patch set 7:<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/11898">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/11898/7/sql/hlr_index.sql">File sql/hlr_index.sql:</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/11898/7/sql/hlr_index.sql@3">Patch Set #7, Line 3:</a> <code style="font-family:monospace,monospace">CREATE UNIQUE INDEX idx_subscr_imsi ON subscriber (imsi);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we're now only using the hlr.sql file when the db is empty; what is the reason to keep this file separate? There is none, right, or am I missing something?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11898/7/src/db.c">File src/db.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/11898/7/src/db.c@243">Patch Set #7, Line 243:</a> <code style="font-family:monospace,monospace">static bool db_is_bootstrapped_v0(struct db_context *dbc)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is still kind of a mix between "is_bootstrapped" and "is_correct".</p><p style="white-space: pre-wrap; word-wrap: break-word;">What if a future db scheme would, for example, drop one of those tables? Since you're always calling this function in db_open() that would need revisiting here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think just checking for 'subscriber' presence to distinguish between v0-empty and v0-bootstrapped would have identical use with less tails attached.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11898/7/src/db.c@346">Patch Set #7, Line 346:</a> <code style="font-family:monospace,monospace">        if (!db_is_bootstrapped_v0(dbc)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">let's break it down:</p><p style="white-space: pre-wrap; word-wrap: break-word;">empty:</p><ul><li>user_version set to 0</li><li>no table 'subscriber' present</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">v0:</p><ul><li>user_version is set to 0</li><li>'subscriber' present</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">vN:</p><ul><li>user_version is set to N</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">So, IIUC, checking table presence makes sense *only* if the user_version is set to 0, to distinguish between "empty" and "v0 bootstrapped"?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Hence I would expect first getting the user_version, then iff zero check db table presence (and it would suffice to check presence of only table 'subscriber', IMHO).</p><p style="white-space: pre-wrap; word-wrap: break-word;">If then the verdict is "empty", go for bootstrapping?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11898/7/src/hlr.c">File src/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/11898/7/src/hlr.c@651">Patch Set #7, Line 651:</a> <code style="font-family:monospace,monospace">      }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(For a unit test, it would be helpful to exit the program right after a db upgrade, so first up I thought, the --upgrade-db option should exit right away. Then again opening up the possibility to automatically upgrade the db when hlr service gets restarted would imply to not exit. So maybe the unit test use case needs a separate option like --check-db-and-exit. i.e. nm)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11898/7/src/hlr_db_tool.c">File src/hlr_db_tool.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/11898/7/src/hlr_db_tool.c@64">Patch Set #7, Line 64:</a> <code style="font-family:monospace,monospace">    printf("  -U --upgrade-db            Allow HLR database schema upgrades.\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(the variables and other options are all called 'db-foo', this is the only one called 'foo-db')</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11898">change 11898</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/11898"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I8aeaa9a404b622657cbc7138106f38aa6ad8d01b </div>
<div style="display:none"> Gerrit-Change-Number: 11898 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <ssperling@sysmocom.de> </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: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 03 Dec 2018 13:47:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>