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 <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: Wed, 27 Nov 2019 15:00:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
