osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/16795 )
Change subject: ss7: Introduce APIs to manage asp_peer hosts ...................................................................... Patch Set 1: (8 comments) https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1//COMMIT_MSG@9 PS1, Line 9: coe (code) https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/include/osmocom/sigtran/osmo_ss7.h File include/osmocom/sigtran/osmo_ss7.h: https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/include/osmocom/sigtran/osmo_ss7.h@432 PS1, Line 432: local_hosts (How about "hosts" instead of "local_hosts" and "host_cnt" instead of "local_host_cnt" (as in osmo_ss7.c)? (same in line below)) https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/include/osmocom/sigtran/osmo_ss7.h@432 PS1, Line 432: int osmo_ss7_asp_peer_set_hosts(struct osmo_ss7_asp_peer *peer, void *talloc_ctx, const char* const* local_hosts, size_t local_host_cnt); (line is > 100 characters) https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/src/osmo_ss7.c File src/osmo_ss7.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/src/osmo_ss7.c@1116 PS1, Line 1116: int osmo_ss7_asp_peer_set_hosts(struct osmo_ss7_asp_peer *peer, void *talloc_ctx, const char* const* hosts, size_t host_cnt) (line > 100 characters) https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/src/osmo_ss7.c@1135 PS1, Line 1135: (empty line should not be there) https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/src/osmo_ss7.c@1141 PS1, Line 1141: /* Makes no sense to have INET_ANY and specific addresses in the set */ : for (i = 0; i < peer->host_cnt; i++) { : iter_is_any = !peer->host[i] || : !strcmp(peer->host[i], "0.0.0.0"); : if (new_is_any && iter_is_any) : return -EINVAL; : if (!new_is_any && iter_is_any) : return -EINVAL; : } : /* Makes no sense to have INET_ANY many times */ : if (new_is_any && peer->host_cnt) : return -EINVAL; (The logic here seems flawed / can be simplified (e.g. move the if (new_is_any ... check up, and we may not even need the for loop if there can only be one INET_ANY address in peer?)... but as I'm writing this, I realize that you just moved the code. So it's fine to leave it as-is in this patch.) https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/src/osmo_ss7.c@2036 PS1, Line 2036: rc This changes the logic; in the previous version, "osmo_stream_srv_link_set_addrs" would not get executed if the ARRAY_SIZE check failed. Is this on purpose, and if so, why not in a separate commit? https://gerrit.osmocom.org/c/libosmo-sccp/+/16795/1/src/osmo_ss7.c@2046 PS1, Line 2046: rc same here -- To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/16795 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-sccp Gerrit-Branch: master Gerrit-Change-Id: I4af2a6915ac57c7baa67343bd9414c65154d67f7 Gerrit-Change-Number: 16795 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Mon, 13 Jan 2020 10:38:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
