neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16202 )
Change subject: add libosmo-mslookup abstract client ...................................................................... Patch Set 19: (10 comments) thanks for the review and sorry for missing it earlier... 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. […] note that the n is sizeof(), i.e. the maximum buffer size, and not the strlen(). This is merely memory sanitation. We are still comparing MSISDNs in full length in all cases. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@130 PS15, Line 130: return 0; > Rather assert() here. we don't know where the id came from, maybe it is random data incoming from the internet. We should absolutely not assert. We could argue whether an unknown ID type should always return not-equal. But at this point we already know that a->type == b->type, so both having an unknown ID type would evaluate as "both are unset/empty/invalid", hence I chose to return 0, in the sense of cmp. 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... 64 is just the initial buffer size, if it needs to be longer, a longer buffer will get allocated in a second printf pass. (Most IPv6 addresses I see end up something like aaaa:bbbb:cccc:dddd::23, i.e. shortened) https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@253 PS15, Line 253: 10 > EINVAL these numbers are a continuation of returning -1..-9 in osmo_mslookup_query_init_from_domain_str(), mostly for debugging: find out which part of the code path didn't like the token. (This is a static function, basically part of osmo_mslookup_query_init_from_domain_str()) 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. […] it must be a pointer to a caller allocated structure, there is no other way to use this function signature. But you shall have API comments. 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 not fancy at all, it is the way to tell the compiler to zero initialize all struct members, which I use all the time. IMO memset() is a hack to work around not using a proper compiler directive. I explicitly want *q to be zero initialized also if parsing failed, as a second safeguard to not use random data. Granted, on some failure modes we return a partially populated struct ... but at least no random data that could cause buffer overflows. 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 this is a static function to find an entry, which may then be modified: no 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. […] I use talloc_zero() by principle, and in my coding style I often fully initialize all members shortly after: I would rather initialize twice than introduce non-obvious missing initialization, e.g. if the struct grows more members in the future. It is unlikely to be any measurable performance impact. 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). true and that's fine? https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client_fake.c@136 PS15, Line 136: talloc_zero > same true and that's fine? -- 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: 19 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: Mon, 06 Jan 2020 17:45:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <axilira...@gmail.com> Gerrit-MessageType: comment
