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

[fixeria]

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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 28 Nov 2019 17:01:35 +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/20191128/58a5dba6/attachment.htm>