<p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202">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/+/16202/9/include/osmocom/mslookup/mdns_sock.h">File include/osmocom/mslookup/mdns_sock.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/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@30">Patch Set #9, Line 30:</a> <code style="font-family:monospace,monospace"> int (*cb)(struct osmo_fd *fd, unsigned int what),</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">> the API doc in mdns_sock.c hints at it, but I'm adding a comment there. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(sorry for confusion, I meant a gerrit review comment)</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/9/include/osmocom/mslookup/mdns_sock.h@32">Patch Set #9, Line 32:</a> <code style="font-family:monospace,monospace">int osmo_mdns_sock_send(const struct osmo_mdns_sock *mdns_sock, struct msgb *msg);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Looking at sendto being used in libosmocore, we don't implicitly close the socket. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">does feel slightly weird to get a const of something used to send a message,<br>but it is in fact appropriate here. +1</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup.h">File include/osmocom/mslookup/mslookup.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/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup.h@113">Patch Set #9, Line 113:</a> <code style="font-family:monospace,monospace">int osmo_mslookup_query_from_domain_str(struct osmo_mslookup_query *q, const char *domain);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Done (Neels renamed it, just replying so I can mark this as "resolved" in gerrit. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(btw, I never bother with those "Resolved" tick marks in gerrit at all... they sometimes get set automatically, apparently, but I feel like we don't need to pay attention to them)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c">File src/mslookup/mdns.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/9/src/mslookup/mdns.c@127">Patch Set #9, Line 127:</a> <code style="font-family:monospace,monospace"> osmo_sockaddr_str_from_32(sockaddr_str, *(uint32_t *)rec->data, 0);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">osmo_sockaddr_str_from_32 expects the input to be network-byte-order. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pespin's point here is that the beginning of the uint32_t may be at a memory address that doesn't match a word aligned position. So the CPU expects a uint32_t to start at an address that is a multiple of 4, which is why we need to first copy the four bytes to a local variable before feeding to osmo_sockaddr_str_from_32().</p><p style="white-space: pre-wrap; word-wrap: break-word;">Might even qualify for an osmo_sockaddr_str_from_32p() as in a pointer which takes care of aligning? I guess _from_32() should have taken a pointer as argument from the start :/</p><p style="white-space: pre-wrap; word-wrap: break-word;">For a moment there I thought passing as a uint32_t argument to a func should imply copying; checking:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> #include <stdio.h><br> #include <stdint.h><br> uint8_t bytes[8] = { 1, 2, 3, 4, 5, 6, 7, 8 };<br> void uint32_func(uint32_t val)<br> {<br> printf("%04x\n", val);<br> }<br> int main()<br> {<br> int i;<br> for (i = 0; i < 4; i++)<br> uint32_func(*(uint32_t*)(&bytes[i]));<br> return 0;<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">gcc -o x -fsanitize=address -fsanitize=undefined x.c</p><p style="white-space: pre-wrap; word-wrap: break-word;">./x</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> 4030201<br> x.c:15:17: runtime error: load of misaligned address 0x5609c31d00a1 for type 'uint32_t', which requires 4 byte alignment<br> 0x5609c31d00a1: note: pointer points here<br> 00 00 00 01 02 03 04 05 06 07 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00<br> ^ <br> 5040302<br> 6050403<br> 7060504</pre><p style="white-space: pre-wrap; word-wrap: break-word;">So we need to align.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_msg.c">File src/mslookup/mdns_msg.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/9/src/mslookup/mdns_msg.c@114">Patch Set #9, Line 114:</a> <code style="font-family:monospace,monospace"> if (osmo_mdns_rfc_record_encode(ctx, msg, &rec) != 0)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm a bit undecided about this, since all osmo_mdns_rfc_*_encode functions with a msgb argument are […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's keep the name "_encode", not add "_append". For exactly those examples that you show; no other function encoding to the end of a msgb has "append" in its name.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c">File src/mslookup/mdns_record.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/9/src/mslookup/mdns_record.c@35">Patch Set #9, Line 35:</a> <code style="font-family:monospace,monospace"> ret->data = (uint8_t *)talloc_asprintf(ctx, "%c%s=%s", (char)len, key, value);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Yes, I'm sure that this is correct. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe they were writing the DNS PoC in Turbo Pascal or something.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/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/9/src/mslookup/mslookup.c@148">Patch Set #9, Line 148:</a> <code style="font-family:monospace,monospace"> if (result->host_v4.ip[0]) {</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> NULL character </pre></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">You mean the nul character :)</p><p style="white-space: pre-wrap; word-wrap: break-word;">We won't get around pointer or array arithmetic, after all it is a pointer to a char array.<br>I've gotten "what's this?" review before for</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (*str)</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and that's why I used</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (str[0])</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Would you prefer an osmo_str_is_empty(str) macro?<br>IMHO we don't need that and C string logic is common enough that we can do either *str or str[0] or *str != '\0' by author's preference.</p><p style="white-space: pre-wrap; word-wrap: break-word;">IMHO this code is obviously printing a v4 addr when it is set, I'm leaving it like it is...</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: 11 </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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 27 Nov 2019 02:17:00 +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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>