<p style="white-space: pre-wrap; word-wrap: break-word;">I really got fed up of reviewing this commit, it's a monster. There's tons of new APIs, objects, and different layers. I could not find a good explanation on the relations so it's almost impossible not getting lost. IIUC there's several layers of objects using lower layers. Please provide separate commits with an introduction for each layer and how it will be used by other objects and which objects will it use.</p><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202">View Change</a></p><p>25 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 style="white-space: pre-wrap; word-wrap: break-word;">Does it make sense to make it a separate shared library if it's only used by osmo-hlr? are we sure it will be used by someone else (osmo-msc)?</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 style="white-space: pre-wrap; word-wrap: break-word;">${shlibs:Depends} can be dropped I think. Maybe it needs to be ${misc:Depends} from looking at line 52.</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 style="white-space: pre-wrap; word-wrap: break-word;">Guinness record of longest log category name. Please shrink it.</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 style="white-space: pre-wrap; word-wrap: break-word;">Looks like this file and mdns.h can be merged.</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 style="white-space: pre-wrap; word-wrap: break-word;">header missing for struct addrinfo.</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 style="white-space: pre-wrap; word-wrap: break-word;">which kind of callback is this one? osmo_fd_cb?</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 style="white-space: pre-wrap; word-wrap: break-word;">const mdns_sock? strange since I'd expect send() returning error means socket must be closed and set to -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@29">Patch Set #9, Line 29:</a> <code style="font-family:monospace,monospace">#define OSMO_MSLOOKUP_SERVICE_HLR_GSUP "gsup.hlr"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We may want a str_value here instead</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 style="white-space: pre-wrap; word-wrap: break-word;">iiuc that's initializing the struct and not finding it. In that case osmo_mslookup_query_init_from_domain_str seems more explicit.</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 style="white-space: pre-wrap; word-wrap: break-word;">Is this internal stuff then? Are we sure we are not installing it with autotools then?</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 style="white-space: pre-wrap; word-wrap: break-word;">There's no allocator/constructor for struct osmo_mslookup_client_method ? Looks like each implementation has its own constructor? Please document so here.</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 style="white-space: pre-wrap; word-wrap: break-word;">is -losmogsm really 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@76">Patch Set #9, Line 76:</a> <code style="font-family:monospace,monospace"> msgb_free(msg);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">missing "talloc_free(req.domain);" here?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Better use goto for ordered cleanup.</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@86">Patch Set #9, Line 86:</a> <code style="font-family:monospace,monospace"> * NULL on error.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need for new 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/+/16202/9/src/mslookup/mdns.c@106">Patch Set #9, Line 106:</a> <code style="font-family:monospace,monospace"> if (req)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this if is not needed.</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@108">Patch Set #9, Line 108:</a> <code style="font-family:monospace,monospace"> if (query)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this if is not needed.</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@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 style="white-space: pre-wrap; word-wrap: break-word;">use osmo_loadbe or whatever here, you may incurr into unaligned access?</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@92">Patch Set #9, Line 92:</a> <code style="font-family:monospace,monospace"> * -EINVAL on error.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">new line not needed.</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_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 style="white-space: pre-wrap; word-wrap: break-word;">this looks like function should be called encode_append or alike.</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 style="white-space: pre-wrap; word-wrap: break-word;">are you sure "len" used here is correct?</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_record.c@39">Patch Set #9, Line 39:</a> <code style="font-family:monospace,monospace"> ret->length = len + 1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">and here.</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_record.c@45">Patch Set #9, Line 45:</a> <code style="font-family:monospace,monospace"> * NULL on error.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">all around: new line not 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_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 style="white-space: pre-wrap; word-wrap: break-word;">wtf? is it really worth memcpying everything then changing tons of stuff in place?</p><p style="white-space: pre-wrap; word-wrap: break-word;">All this is a bit strange imho, you are using packed structs with host-endian values...</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 style="white-space: pre-wrap; word-wrap: break-word;">"was sent" or "is to be sent" / "can be sent"?</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 style="white-space: pre-wrap; word-wrap: break-word;">what are you actually checking here? if First byte/octet in address is not zero? why?</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: 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 18:32:38 +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>