dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-iuh/+/26722 )
Change subject: iu_helpers: make new_transp_info_(rtp|gtp) public ...................................................................... Patch Set 4: (8 comments) https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/iu_helpers.c File src/iu_helpers.c: https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/iu_helpers.c@134 PS3, Line 134: switch (addr->u.sin.sin_family) { > you should better use u.sa here conceptually, since you don't really know > whether it's AF_INET (u. […] Done https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/iu_helpers.c@140 PS3, Line 140: ip_len = sizeof(addr->u.sin6.sin6_addr.s6_addr); > Not sure if this is correct or needs to be sizeof(addr->u.sin6.sin6_addr) or > it doesn't matter. This should be uint8_t s6_addr[16], so its correct I think. https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/iu_helpers.c@148 PS3, Line 148: len = 160 / 8; > can you explain/document these numbers? Its a fixed size, x213 supports a ton of different address formats. I have now put the references to the various sections and tables in the code. https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/iu_helpers.c@183 PS3, Line 183: binding_id[0] = port & 0xff; > probably osmo_store16be() would be more clear here. Done https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/iu_helpers.c@187 PS3, Line 187: new_transp_layer_addr(&tli->transportLayerAddress, addr, use_x213_nsap); > check return code an return NULL Done https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/iu_helpers.c@201 PS3, Line 201: new_transp_layer_addr(&tli->transportLayerAddress, addr, use_x213_nsap); > check return code an return NULL Done https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/ranap_msg_factory.c File src/ranap_msg_factory.c: https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/ranap_msg_factory.c@753 PS3, Line 753: rtp_addr.u.sin.sin_addr.s_addr = htonl(rtp_ip); > are you sure the rtp_ip is passed above in host byte order? looks weird to me. Yes, I think this is correct and it also works in real life too. can you have a alook at test-helpers.c, there I am using inet_pton to initialize the value and the output is correct. As far as I am aware sin_port and sin_addr are in network byte order. Otherwise it also wouln't integrate with sockaddr_str. https://gerrit.osmocom.org/c/osmo-iuh/+/26722/3/src/ranap_msg_factory.c@826 PS3, Line 826: gtp_addr.u.sin.sin_addr.s_addr = htonl(gtp_ip); > same Done -- To view, visit https://gerrit.osmocom.org/c/osmo-iuh/+/26722 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-iuh Gerrit-Branch: master Gerrit-Change-Id: I1e369718de8c4c7db1f1af1e6864562164ada6cf Gerrit-Change-Number: 26722 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Fri, 07 Jan 2022 16:11:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
