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