Change in osmo-hlr[master]: add libosmo-mslookup and mDNS implementation

historical

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 <info at 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 <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: Mon, 25 Nov 2019 21:46:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191125/0d268df1/attachment.htm>