This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
osmith gerrit-no-reply at lists.osmocom.orgosmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16202 )
Change subject: add libosmo-mslookup and mDNS implementation
......................................................................
Patch Set 10:
(29 comments)
Thanks for the reviews! I've pushed an update to address everything, except for splitting this up in multiple patches.
> 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.
I'll do that tomorrow.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/configure.ac
File configure.ac:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/configure.ac@182
PS9, Line 182: libosmo-mslookup.pc
> Yes. We want to make mslookup integrate-able by any client programs. […]
In particular, it would make sense for OsmoMSC in combination with SMS over GSUP.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/debian/control
File debian/control:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/debian/control@75
PS9, Line 75: Depends: ${shlibs:Depends},
> (@osmith? ... […]
Ack
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/hlr/logging.h
File include/osmocom/hlr/logging.h:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/hlr/logging.h@11
PS9, Line 11: DMSLOOKUP,
> Using this name isn't really a problem, is it? Any abbreviation I can think of would make it hard to […]
Changed to DMSLKP.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h
File include/osmocom/mslookup/mdns_sock.h:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@1
PS9, Line 1: /* Copyright 2019 by sysmocom s.f.m.c. GmbH <info at sysmocom.de>
> Looks like this file and mdns.h can be merged.
mdns_sock is just about the socket related functions, while mdns.h is part of the stack that builds the mdns packets (therefore I'd rather not merge the two files):
* mdns.h: encoding and decoding mslookup query/result
* mdns_msg.h/mdns_record.h: encoding and decoding whole dns packets (request/answer); answer consists of multiple records
* mdns_rfc.h: encoding and decoding sections of dns packets (header, question/record) and qname
mdns_msg.h and mdns_record.h will be merged, as discussed with Neels.
I'll split this patch up, to introduce one layer at a time. Then this hopefully makes more sense, and it also makes reviewing easier.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@26
PS9, Line 26: struct addrinfo *ai;
> ah, you mean the #include
Added #include <netdb.h>.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@30
PS9, Line 30: int (*cb)(struct osmo_fd *fd, unsigned int what),
> the API doc in mdns_sock.c hints at it, but I'm adding a comment there.
Couldn't find your update (maybe not committed?), but I've updated the apidoc comment to:
* \param[in] cb callback for incoming data that will be passed to osmo_fd_setup (should read from osmo_fd->fd).
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@32
PS9, Line 32: int osmo_mdns_sock_send(const struct osmo_mdns_sock *mdns_sock, struct msgb *msg);
> (it doesn't close the socket implicitly. […]
Looking at sendto being used in libosmocore, we don't implicitly close the socket. Instead we return sendto's return code. But the callers never close the socket on error from what I can find:
* https://git.osmocom.org/libosmocore/tree/src/gb/gprs_ns.c?id=8a7eed50dbd7fc05a1c3bbf302ef8e42a5698a98#n2012
* https://git.osmocom.org/libosmocore/tree/src/gb/gprs_ns_frgre.c?id=8a7eed50dbd7fc05a1c3bbf302ef8e42a5698a98#n142
* https://git.osmocom.org/libosmocore/tree/src/gb/gprs_ns_frgre.c?id=8a7eed50dbd7fc05a1c3bbf302ef8e42a5698a98#n306
* https://git.osmocom.org/libosmocore/tree/src/stats.c?id=8a7eed50dbd7fc05a1c3bbf302ef8e42a5698a98#n459
This means, not doing it implicitly would follow what we are doing already.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup.h
File include/osmocom/mslookup/mslookup.h:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup.h@29
PS9, Line 29: #define OSMO_MSLOOKUP_SERVICE_HLR_GSUP "gsup.hlr"
> We could in fact also drop them, except for the "gsup. […]
Done (Neels removed them)
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup.h@113
PS9, Line 113: int osmo_mslookup_query_from_domain_str(struct osmo_mslookup_query *q, const char *domain);
> ack
Done (Neels renamed it, just replying so I can mark this as "resolved" in gerrit. Same with following "Done" comments, if Neels already replied with "ack" etc., he fixed them and I'm just marking it as resolved here so I know what is still open.)
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup_client.h
File include/osmocom/mslookup/mslookup_client.h:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup_client.h@35
PS9, Line 35: /*! This part of a lookup request is not seen by the individual query method implementations. */
> adjusting API doc....
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup_client.h@95
PS9, Line 95: struct osmo_mslookup_client_method {
> ack
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/libosmo-mslookup.pc.in
File libosmo-mslookup.pc.in:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/libosmo-mslookup.pc.in@9
PS9, Line 9: Libs: -L${libdir} @TALLOC_LIBS@ -losmogsm -losmo-mslookup -losmocore
> hmm, checking... […]
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c
File src/mslookup/mdns.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@1
PS9, Line 1: /* mslookup specific functions for encoding and decoding mslookup queries/results into mDNS packets, using the high
> (leaving this file for osmith)
Ack
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@76
PS9, Line 76: msgb_free(msg);
> missing "talloc_free(req.domain);" here? […]
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@86
PS9, Line 86: * NULL on error.
> No need for new line.
Removed this new line and similar ones from all the \returns apidocs added in this patch.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@98
PS9, Line 98: query = talloc_zero(ctx, struct osmo_mslookup_query);
> should OSMO_ASSERT(query) here
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@106
PS9, Line 106: if (req)
> this if is not needed.
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@108
PS9, Line 108: if (query)
> this if is not needed.
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@127
PS9, Line 127: osmo_sockaddr_str_from_32(sockaddr_str, *(uint32_t *)rec->data, 0);
> use osmo_loadbe or whatever here, you may incurr into unaligned access?
osmo_sockaddr_str_from_32 expects the input to be network-byte-order.
https://git.osmocom.org/libosmocore/tree/src/sockaddr_str.c?id=8a7eed50dbd7fc05a1c3bbf302ef8e42a5698a98#n258
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_msg.c
File src/mslookup/mdns_msg.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_msg.c@92
PS9, Line 92: * -EINVAL on error.
> new line not needed.
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_msg.c@114
PS9, Line 114: if (osmo_mdns_rfc_record_encode(ctx, msg, &rec) != 0)
I'm a bit undecided about this, since all osmo_mdns_rfc_*_encode functions with a msgb argument are appending to the msgb. Should I call all of them _encode_append then?
I found some existing functions in libosmocore too, which are appending to a msgb, but are also just called _encode(), not _encode_append():
* gsup.c: encode_pdp_info, encode_auth_info, osmo_gsup_encode_an_apdu
* gsm48_ie.c: gsm48_encode_bearer_cap, gsm48_encode_cccap, gsm48_encode_called, ...
So with these findings, I'm leaning more towards keeping the encode() name (as it is more consistent) unless there is a good reason to change it.
I'll update the apidocs of these osmo_mdns_rfc_*_encode functions to make clear, that they are appending to msgb.
> (it encodes a single record, makes sense to me)
What makes sense to you, what Pau proposed or how it was in the patch?
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c
File src/mslookup/mdns_record.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c@35
PS9, Line 35: ret->data = (uint8_t *)talloc_asprintf(ctx, "%c%s=%s", (char)len, key, value);
Yes, I'm sure that this is correct.
https://tools.ietf.org/html/rfc1035#section-3.3.14
> 3.3.14. TXT RDATA format
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
/ TXT-DATA /
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
>
> where:
>
> TXT-DATA One or more <character-string>s.
and
https://tools.ietf.org/html/rfc1035#section-3.3
> <character-string> is a single
length octet followed by that number of characters. <character-string>
is treated as binary information, and can be up to 256 characters in
length (including the length octet).
(Which makes the length information redundant, because the resource record already has a defined length... not sure what they were thinking. -- https://tools.ietf.org/html/rfc1035#section-4.1.3 )
I had this wrong at first, because I did not look up the definition of <character-string> and thought it was just a regular string, without the length byte in front. But it became obvious in wireshark, and there it also displays correctly after I've implemented <character-string> properly.
I'll add a more descriptive comment, and a max length check.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c@39
PS9, Line 39: ret->length = len + 1;
> and here.
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c@45
PS9, Line 45: * NULL on error.
> all around: new line not needed.
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_rfc.c
File src/mslookup/mdns_rfc.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_rfc.c@122
PS9, Line 122: osmo_store16be(buf->id, &buf->id);
> wtf? is it really worth memcpying everything then changing tons of stuff in place?
The struct osmo_mdns_rfc_header has bitfields and uint16:
struct osmo_mdns_rfc_header {
#if OSMO_IS_LITTLE_ENDIAN
uint16_t id;
uint8_t rd:1,
tc:1,
aa:1,
opcode:4,
qr:1; /* QR (0: query, 1: response) */
uint8_t rcode:4,
z:3,
ra:1;
uint16_t qdcount; /* Number of questions */
uint16_t ancount; /* Number of answers */
uint16_t nscount; /* Number of authority records */
uint16_t arcount; /* Number of additional records */
#elif OSMO_IS_BIG_ENDIAN
...
#endif
} __attribute__ ((packed));
I'm doing the memcpy to fill out the bitfields correctly, and osmo_store16be to fix up the uint16_ts.
> This should suffice:
This does not work, because then the bytes in uint16_t (id, qdcount, ...) are swapped.
Wireshark shows 256 questions (question sections) instead of one.
diff --git a/tests/mslookup/mdns_test.err b/tests/mslookup/mdns_test.err
index 51e5afe6..3cb1ddcb 100644
--- a/tests/mslookup/mdns_test.err
+++ b/tests/mslookup/mdns_test.err
@@ -95,7 +95,7 @@ header in:
.ancount = 0
.nscount = 0
.arcount = 0
-encoded: 05 39 00 00 00 01 00 00 00 00 00 00
+encoded: 39 05 00 00 01 00 00 00 00 00 00 00
header out:
.id = 1337
.qr = 0
@@ -128,7 +128,7 @@ header in:
.ancount = 1111
.nscount = 2222
.arcount = 3333
-encoded: 00 2a 97 a3 04 d2 04 57 08 ae 0d 05
+encoded: 2a 00 97 a3 d2 04 57 04 ae 08 05 0d
header out:
.id = 42
.qr = 1
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_rfc.c@134
PS9, Line 134:
> here just […]
Does not work, see above.
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_sock.c
File src/mslookup/mdns_sock.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_sock.c@40
PS9, Line 40: * will not only be called when someone else is sending data, but also for data that was sent from this osmo_mdns_sock.
> the point is that a multicast socket receives everything, also what it sends itself, so as soon as y […]
Done
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_sock.c@44
PS9, Line 44: * \param[in] cb callback for incoming data (should read from osmo_fd->fd).
I have this now, which should make it obvious that it becomes "struct osmo_fd"'s callback:
> * \param[in] cb callback for incoming data that will be passed to osmo_fd_setup (should read from osmo_fd->fd).
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mslookup.c
File src/mslookup/mslookup.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mslookup.c@148
PS9, Line 148: if (result->host_v4.ip[0]) {
> host_v4.ip is a char[], it is checking for an empty string.
Ack
--
To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/16202
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I83487ab8aad1611eb02e997dafbcb8344da13df1
Gerrit-Change-Number: 16202
Gerrit-PatchSet: 10
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Tue, 26 Nov 2019 16:03:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: neels <nhofmeyr at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191126/890e07dc/attachment.htm>