Change in osmo-hlr[master]: add libosmo-mslookup abstract client

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16202 )

Change subject: add libosmo-mslookup abstract client
......................................................................


Patch Set 14:

(9 comments)

https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c 
File src/mslookup/mslookup.c:

https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@52 
PS14, Line 52:  *   src/mslookup/mslookup_client_mdns.c   lookup method implementing multicast DNS, client side.
This line should go in next patches but fine.


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@56 
PS14, Line 56:  *   src/mslookup/mslookup_client_fake.c   lookup method generating fake results, for testing client implementations.
Better put this one after mslookup_client.c


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@58 
PS14, Line 58:  *   src/mslookup/mdns*.c                  implementation of DNS to be used by mslookup_client_mdns.c,
You may want to put that inside (1), before (1a) since it's for both 1a and 1b.


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@61 
PS14, Line 61:  *   contrib/dgsm/esme_dgsm.py                 Example implementation for an mslookup enabled SMS handler.
utils/?


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@80 
PS14, Line 80:  *  src/proxy.c       Keep track of proxied IMSIs and cache important subscriber data.
is code in this file going to be used outside of dgsm scope? if not, sounds like it should be renamed to dgsm_proxy.c or dgsm_cache.c or alike.


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@91 
PS14, Line 91: #define CMP(a,b) (a < b? -1 : (a > b? 1 : 0))
Don't we have an OSMO_CMP somewhere for that? BTW, missing () around a and b. Missing space after b.


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@214 
PS14, Line 214: 			if (result->host_v4.ip[0]) {
!= '\0'. I had to go look into the header then into "struct osmo_sockaddr_str" @ libosmocore to find if that was a string or some kind of pointer.


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@218 
PS14, Line 218: 			if (result->host_v6.ip[0]) {
same


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup_client.c 
File src/mslookup/mslookup_client.c:

https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup_client.c@72 
PS14, Line 72: 	if (!client)
do we really expect someone to call this function with a null pointer? I wouldn't expect it. Add an assert if you are afraid about bad use.



-- 
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: 14
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: Wed, 27 Nov 2019 14:09:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191127/dda3e410/attachment.htm>