<p style="white-space: pre-wrap; word-wrap: break-word;">thanks for the review and sorry for missing it earlier...</p><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202">View Change</a></p><p>10 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/+/16202/15/src/mslookup/mslookup.c">File src/mslookup/mslookup.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/+/16202/15/src/mslookup/mslookup.c@128">Patch Set #15, Line 128:</a> <code style="font-family:monospace,monospace">strncmp(a->msisdn, b->msisdn, sizeof(a->msisdn));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What if a->msisdn is e.g. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">note that the n is sizeof(), i.e. the maximum buffer size, and not the strlen(). This is merely memory sanitation. We are still comparing MSISDNs in full length in all cases.</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/+/16202/15/src/mslookup/mslookup.c@130">Patch Set #15, Line 130:</a> <code style="font-family:monospace,monospace">return 0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Rather assert() here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">we don't know where the id came from, maybe it is random data incoming from the internet. We should absolutely not assert. We could argue whether an unknown ID type should always return not-equal. But at this point we already know that a->type == b->type, so both having an unknown ID type would evaluate as "both are unset/empty/invalid", hence I chose to return 0, in the sense of cmp.</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/+/16202/15/src/mslookup/mslookup.c@232">Patch Set #15, Line 232:</a> <code style="font-family:monospace,monospace">64</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Given that IPv6 addresses can be quite long, I would use at least 128...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">64 is just the initial buffer size, if it needs to be longer, a longer buffer will get allocated in a second printf pass.<br>(Most IPv6 addresses I see end up something like aaaa:bbbb:cccc:dddd::23, i.e. shortened)</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/+/16202/15/src/mslookup/mslookup.c@253">Patch Set #15, Line 253:</a> <code style="font-family:monospace,monospace">10</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">EINVAL</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">these numbers are a continuation of returning -1..-9 in osmo_mslookup_query_init_from_domain_str(), mostly for debugging: find out which part of the code path didn't like the token. (This is a static function, basically part of osmo_mslookup_query_init_from_domain_str())</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/+/16202/15/src/mslookup/mslookup.c@266">Patch Set #15, Line 266:</a> <code style="font-family:monospace,monospace">struct osmo_mslookup_query *q</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Parameters need to be documented. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">it must be a pointer to a caller allocated structure, there is no other way to use this function signature. But you shall have API comments.</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/+/16202/15/src/mslookup/mslookup.c@274">Patch Set #15, Line 274:</a> <code style="font-family:monospace,monospace">*q = (struct osmo_mslookup_query){};</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So this is a fancy way to initialize a caller-allocated structure, right? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">IMHO not fancy at all, it is the way to tell the compiler to zero initialize all struct members, which I use all the time. IMO memset() is a hack to work around not using a proper compiler directive.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I explicitly want *q to be zero initialized also if parsing failed, as a second safeguard to not use random data.<br>Granted, on some failure modes we return a partially populated struct ... but at least no random data that could cause buffer overflows.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client.c">File src/mslookup/mslookup_client.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/+/16202/15/src/mslookup/mslookup_client.c@45">Patch Set #15, Line 45:</a> <code style="font-family:monospace,monospace">struct</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">const</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is a static function to find an entry, which may then be modified: no const.</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/+/16202/15/src/mslookup/mslookup_client.c@59">Patch Set #15, Line 59:</a> <code style="font-family:monospace,monospace">talloc_zero</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would avoid zero-initialization here, because 2/3 fields are explicitly initialized below. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I use talloc_zero() by principle, and in my coding style I often fully initialize all members shortly after: I would rather initialize twice than introduce non-obvious missing initialization, e.g. if the struct grows more members in the future. It is unlikely to be any measurable performance impact.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client_fake.c">File src/mslookup/mslookup_client_fake.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/+/16202/15/src/mslookup/mslookup_client_fake.c@59">Patch Set #15, Line 59:</a> <code style="font-family:monospace,monospace">talloc_zero</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Zero-initialization is redundant here (see the next line).</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">true and that's fine?</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/+/16202/15/src/mslookup/mslookup_client_fake.c@136">Patch Set #15, Line 136:</a> <code style="font-family:monospace,monospace">talloc_zero</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">same</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">true and that's fine?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202">change 16202</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/+/16202"/><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: I83487ab8aad1611eb02e997dafbcb8344da13df1 </div>
<div style="display:none"> Gerrit-Change-Number: 16202 </div>
<div style="display:none"> Gerrit-PatchSet: 19 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 06 Jan 2020 17:45:48 +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: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>