fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16202 )
Change subject: add libosmo-mslookup abstract client ...................................................................... Patch Set 15: (12 comments) https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c File src/mslookup/mslookup.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@128 PS15, Line 128: strncmp(a->msisdn, b->msisdn, sizeof(a->msisdn)); What if a->msisdn is e.g. '900' while b->msisdn is '900369'? They are not equal, but strncmp() would return 0. I think you either need to compare strlen() too, or just use regular strcmp(). https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@130 PS15, Line 130: return 0; Rather assert() here. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@211 PS15, Line 211: if (result->rc == OSMO_MSLOOKUP_RC_RESULT) Cosmetic: this can be replaced by '} else {'. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@232 PS15, Line 232: 64 Given that IPv6 addresses can be quite long, I would use at least 128... https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@253 PS15, Line 253: 10 EINVAL https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@256 PS15, Line 256: 11 ENOMEM https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@266 PS15, Line 266: struct osmo_mslookup_query *q Parameters need to be documented. At least I would like to know whether it's a pointer to a caller-allocated structure, or do we allocate it here? https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@274 PS15, Line 274: *q = (struct osmo_mslookup_query){}; So this is a fancy way to initialize a caller-allocated structure, right? IMHO, memset(q, 0x00, sizeof(*q)) looks way more cleaner... I would also place this after all checks below (compiler would do this for us). If we return rc != 0, the caller shall consider *q uninitialized. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client.c File src/mslookup/mslookup_client.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client.c@45 PS15, Line 45: struct const https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client.c@59 PS15, Line 59: talloc_zero I would avoid zero-initialization here, because 2/3 fields are explicitly initialized below. Just add 'next_request_handle = 0', so we would avoid a redundant write cycle. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client_fake.c File src/mslookup/mslookup_client_fake.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client_fake.c@59 PS15, Line 59: talloc_zero Zero-initialization is redundant here (see the next line). https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client_fake.c@136 PS15, Line 136: talloc_zero same -- 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: 15 Gerrit-Owner: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <axilira...@gmail.com> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Thu, 28 Nov 2019 17:01:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
