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

[neels]

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

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


Patch Set 14:

(6 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.
(this was explicitly meant as an overview across all upcoming patches)


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/?
IMHO it still belongs in contrib


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 rename […]
D-GSM is the concept for finding out where to proxy to, the GSUP proxy itself is a building block not necessarily limited to D-GSM


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.
ah indeed


https://gerrit.osmocom.org/c/osmo-hlr/+/16202/14/src/mslookup/mslookup.c@214 
PS14, Line 214: 			if (result->host_v4.ip[0]) {
> != '\0'. […]
this is literally the least interesting aspect of these patches that we could possibly discuss :P


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. […]
no, I want to return false if the client is not allocated, not abort the program.

 osmo_mslookup_client_active(g_hlr->mslookup.client.running);



-- 
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 15:00:01 +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/20191127/9aa7168c/attachment.htm>