neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16202 )
Change subject: add libosmo-mslookup and mDNS implementation ...................................................................... Patch Set 9: (21 comments) 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 > Does it make sense to make it a separate shared library if it's only used by > osmo-hlr? are we sure i […] Yes. We want to make mslookup integrate-able by any client programs. The osmo-mslookup-client utility is just one particular variant of it, and the python esme and dialplan are mere proof-of-concept examples. Any client written in C and already using the libosmocore select loop would obviously want to use the library directly. 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}, > ${shlibs:Depends} can be dropped I think. […] (@osmith? ... I have no idea about this) 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, > Guinness record of longest log category name. Please shrink it. 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? 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 <i...@sysmocom.de> > Looks like this file and mdns.h can be merged. @osmith, I also think that there could/should be less files about the mDNS proto itself https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@26 PS9, Line 26: struct addrinfo *ai; > header missing for struct addrinfo. ah, you mean the #include 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), > which kind of callback is this one? osmo_fd_cb? struct osmo_fd defines its cb this way, there is no separte osmo_fd_cb_t; the API doc in mdns_sock.c hints at it, but I'm adding a comment there. 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); > const mdns_sock? strange since I'd expect send() returning error means socket > must be closed and set […] (it doesn't close the socket implicitly. should it? @osmith?) 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 may want a str_value here instead We could in fact also drop them, except for the "gsup.hlr" one, which is actually used in the osmo-hlr server. All the others are entirely up to the clients to use, and there isn't actually a C program using these names. 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. 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); > iiuc that's initializing the struct and not finding it. […] ack 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. */ > Is this internal stuff then? Are we sure we are not installing it with > autotools then? adjusting API doc.... https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup_client.h@95 PS9, Line 95: struct osmo_mslookup_client_method { > There's no allocator/constructor for struct osmo_mslookup_client_method ? > Looks like each implementa […] ack 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 > is -losmogsm really needed? hmm, checking... /usr/bin/ld: ./.libs/libosmo-mslookup.so: undefined reference to `osmo_msisdn_str_valid' /usr/bin/ld: ./.libs/libosmo-mslookup.so: undefined reference to `osmo_get_rand_id' /usr/bin/ld: ./.libs/libosmo-mslookup.so: undefined reference to `osmo_imsi_str_valid' Seems that it is in fact needed. 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) 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 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@114 PS9, Line 114: if (osmo_mdns_rfc_record_encode(ctx, msg, &rec) != 0) > this looks like function should be called encode_append or alike. (it encodes a single record, makes sense to me) 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); > are you sure "len" used here is correct? 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: "\x03foo\x07example\x03com" @osmith, the same applies here, right? 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? […] @osmith, I think we talked about this once, maybe I wasn't clear enough... This should suffice: { struct osmo_mdns_rfc_header *buf = (struct osmo_mdns_rfc_header *) msgb_put(msg, sizeof(*hdr)); *buf = *hdr; } https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_rfc.c@134 PS9, Line 134: here just *hdr = *(struct osmo_mdns_rfc_header*)data; return 0; 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. 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. > "was sent" or "is to be sent" / "can be sent"? 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? 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). maybe add "used as struct osmo_fd's cb" 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]) { > what are you actually checking here? if First byte/octet in address is not > zero? why? host_v4.ip is a char[], it is checking for an empty string. -- 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: 9 Gerrit-Owner: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Mon, 25 Nov 2019 21:46:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment