<p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202">View Change</a></p><p>21 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/configure.ac">File configure.ac:</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/configure.ac@182">Patch Set #9, Line 182:</a> <code style="font-family:monospace,monospace"> libosmo-mslookup.pc</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does it make sense to make it a separate shared library if it's only used by osmo-hlr? are we sure i […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes. We want to make mslookup integrate-able by any client programs.<br>The osmo-mslookup-client utility is just one particular variant of it, and the python esme and dialplan are mere proof-of-concept examples.<br>Any client written in C and already using the libosmocore select loop would obviously want to use the library directly.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/debian/control">File debian/control:</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/debian/control@75">Patch Set #9, Line 75:</a> <code style="font-family:monospace,monospace">Depends: ${shlibs:Depends},</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">${shlibs:Depends} can be dropped I think. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(@osmith? ... I have no idea about this)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/hlr/logging.h">File include/osmocom/hlr/logging.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/hlr/logging.h@11">Patch Set #9, Line 11:</a> <code style="font-family:monospace,monospace"> DMSLOOKUP,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Guinness record of longest log category name. Please shrink it.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Using this name isn't really a problem, is it? Any abbreviation I can think of would make it hard to understand what it is about... any suggestions?</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/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@1">Patch Set #9, Line 1:</a> <code style="font-family:monospace,monospace">/* Copyright 2019 by sysmocom s.f.m.c. GmbH <info@sysmocom.de></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Looks like this file and mdns.h can be merged.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">@osmith, I also think that there could/should be less files about the mDNS proto itself</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@26">Patch Set #9, Line 26:</a> <code style="font-family:monospace,monospace"> struct addrinfo *ai;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">header missing for struct addrinfo.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ah, you mean the #include</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@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;">which kind of callback is this one? osmo_fd_cb?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">struct osmo_fd defines its cb this way, there is no separte osmo_fd_cb_t;<br>the API doc in mdns_sock.c hints at it, but I'm adding a comment there.</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;">const mdns_sock? strange since I'd expect send() returning error means socket must be closed and set […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(it doesn't close the socket implicitly. should it? @osmith?)</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@29">Patch Set #9, Line 29:</a> <code style="font-family:monospace,monospace">#define OSMO_MSLOOKUP_SERVICE_HLR_GSUP "gsup.hlr"</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">We may want a str_value here instead</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We could in fact also drop them, except for the "gsup.hlr" one, which is actually used in the osmo-hlr server.<br>All the others are entirely up to the clients to use, and there isn't actually a C program using these names.<br>Service names can be freely invented in osmo-hlr.cfg. So these are little more than suggestions / avoiding typos. I guess let's drop most of them from here. Service name conventions are listed in the D-GSM chapter of the osmo-hlr user manual.</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/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;">iiuc that's initializing the struct and not finding it. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ack</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_client.h">File include/osmocom/mslookup/mslookup_client.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_client.h@35">Patch Set #9, Line 35:</a> <code style="font-family:monospace,monospace">/*! This part of a lookup request is not seen by the individual query method implementations. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is this internal stuff then? Are we sure we are not installing it with autotools then?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">adjusting API doc....</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/mslookup_client.h@95">Patch Set #9, Line 95:</a> <code style="font-family:monospace,monospace">struct osmo_mslookup_client_method {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">There's no allocator/constructor for struct osmo_mslookup_client_method ? Looks like each implementa […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/libosmo-mslookup.pc.in">File libosmo-mslookup.pc.in:</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/libosmo-mslookup.pc.in@9">Patch Set #9, Line 9:</a> <code style="font-family:monospace,monospace">Libs: -L${libdir} @TALLOC_LIBS@ -losmogsm -losmo-mslookup -losmocore</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">is -losmogsm really needed?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">hmm, checking...</p><p style="white-space: pre-wrap; word-wrap: break-word;">/usr/bin/ld: ./.libs/libosmo-mslookup.so: undefined reference to `osmo_msisdn_str_valid'<br>/usr/bin/ld: ./.libs/libosmo-mslookup.so: undefined reference to `osmo_get_rand_id'<br>/usr/bin/ld: ./.libs/libosmo-mslookup.so: undefined reference to `osmo_imsi_str_valid'</p><p style="white-space: pre-wrap; word-wrap: break-word;">Seems that it is in fact needed.</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@1">Patch Set #9, Line 1:</a> <code style="font-family:monospace,monospace">/* mslookup specific functions for encoding and decoding mslookup queries/results into mDNS packets, using the high</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(leaving this file for osmith)</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/src/mslookup/mdns.c@98">Patch Set #9, Line 98:</a> <code style="font-family:monospace,monospace"> query = talloc_zero(ctx, struct osmo_mslookup_query);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">should OSMO_ASSERT(query) here</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;">this looks like function should be called encode_append or alike.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(it encodes a single record, makes sense to me)</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;">are you sure "len" used here is correct?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not sure about TXT records, but DNS query strings actually contain token lengths embedded as uint8_t in the URL string instead of dots:<br>"\x03foo\x07example\x03com"<br>@osmith, the same applies here, right?</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_rfc.c">File src/mslookup/mdns_rfc.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_rfc.c@122">Patch Set #9, Line 122:</a> <code style="font-family:monospace,monospace"> osmo_store16be(buf->id, &buf->id);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">wtf? is it really worth memcpying everything then changing tons of stuff in place? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">@osmith, I think we talked about this once, maybe I wasn't clear enough...<br>This should suffice:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> {<br> struct osmo_mdns_rfc_header *buf = (struct osmo_mdns_rfc_header *) msgb_put(msg, sizeof(*hdr));<br> *buf = *hdr;<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/+/16202/9/src/mslookup/mdns_rfc.c@134">Patch Set #9, Line 134:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">here just</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> *hdr = *(struct osmo_mdns_rfc_header*)data;<br> return 0;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">We almost don't even need this function, then, but the length check is important, and I like it to be in this explicit function.</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_sock.c">File src/mslookup/mdns_sock.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_sock.c@40">Patch Set #9, Line 40:</a> <code style="font-family:monospace,monospace"> * will not only be called when someone else is sending data, but also for data that was sent from this osmo_mdns_sock.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"was sent" or "is to be sent" / "can be sent"?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">the point is that a multicast socket receives everything, also what it sends itself, so as soon as you send(), you'll also get a recv() with your own data back. @osmith, clarify the doc 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/9/src/mslookup/mdns_sock.c@44">Patch Set #9, Line 44:</a> <code style="font-family:monospace,monospace"> * \param[in] cb callback for incoming data (should read from osmo_fd->fd).</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe add "used as struct osmo_fd's cb"</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><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what are you actually checking here? if First byte/octet in address is not zero? why?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">host_v4.ip is a char[], it is checking for an empty string.</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: 9 </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: Mon, 25 Nov 2019 21:46:22 +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"> Gerrit-MessageType: comment </div>