Change in osmo-bsc[master]: oml: encode IPv6 NSVC using the new OML attribute NM_ATT_OSMO_NS_LINK...

[lynxis lazus]

lynxis lazus has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/20234 )

Change subject: oml: encode IPv6 NSVC using the new OML attribute NM_ATT_OSMO_NS_LINK_CFG
......................................................................


Patch Set 10:

(12 comments)

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/9/include/osmocom/bsc/gsm_data.h 
File include/osmocom/bsc/gsm_data.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/9/include/osmocom/bsc/gsm_data.h@12 
PS9, Line 12: #include <osmocom/core/sockaddr_str.h>
> This include is not needed AFAICT.
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/9/src/osmo-bsc/bsc_vty.c 
File src/osmo-bsc/bsc_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/9/src/osmo-bsc/bsc_vty.c@784 
PS9, Line 784: 			osmo_htons(nsvc->remote.u.sin.sin_port), VTY_NEWLINE);
> osmo_sockaddr_port(&nsvc->remote) (and you can also remove the byteswap.h).
See the comment. "Can't use remote.port because it's only valid when family != AF_UNSPEC". But I've also added checked the port to be != 0.


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c 
File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c@206 
PS3, Line 206: 	if (bts->gprs.nsvc->remote.u.sa.sa_family == AF_INET6) {
> switch?
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c@208 
PS3, Line 208: 		osmo_store16be(IPPROTO_IPV6, &buf[0]);
> Please let's use our own define here, not this define which was nothing to do. That's for IPoverIP.
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c@212 
PS3, Line 212: 		memcpy(&buf[4], &bts->gprs.nsvc[0].remote.u.sin6.sin6_port, sizeof(uint16_t));
> Really? memcpy for a uint16_t?
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c@218 
PS3, Line 218: 		memcpy(&buf[0], &bts->gprs.nsvc[0].remote.u.sin.sin_port, sizeof(uint16_t));
> Same
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/4/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c 
File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/4/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c@216 
PS4, Line 216: 		buf[4] = *(((char *) &bts->gprs.nsvc[0].remote.u.sin6.sin6_port) + 0);
> @pespin: do you like it more than a memcpy?
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/5/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c 
File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/5/src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c@216 
PS5, Line 216: 		buf[4] = *(((char *) &bts->gprs.nsvc[0].remote.u.sin6.sin6_port) + 0);
> Indeed, good point, my bad then, feel free to move back to memcpy.
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/pcu_sock.c 
File src/osmo-bsc/pcu_sock.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/pcu_sock.c@199 
PS3, Line 199: 			LOGP(DPCU, LOGL_ERROR, "Can not encode IPv6 NSVC!");
> didn't fixeria already add support for it?
This is PCU IF 9. See other patches in line.


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/3/src/osmo-bsc/pcu_sock.c@205 
PS3, Line 205: 		info_ind->nsvci[i] = nsvc->nsvci;
> You should have 2 iter variable shere, otherwise if you "continue;" above, you end up with holes in  […]
It's not needed. The PCUIF have 2 nsvcs and is zero initialized. So AF_UNSPEC is the default and will be ignored.


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/9/src/osmo-bsc/pcu_sock.c 
File src/osmo-bsc/pcu_sock.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/20234/9/src/osmo-bsc/pcu_sock.c@196 
PS9, Line 196: 		info_ind->remote_port[i] = osmo_ntohs(nsvc->remote.u.sin.sin_port);
> osmo_sockaddr_port()
Ack


https://gerrit.osmocom.org/c/osmo-bsc/+/20234/9/src/osmo-bsc/pcu_sock.c@197 
PS9, Line 197: 		info_ind->remote_ip[i] = osmo_ntohl(nsvc->remote.u.sin.sin_addr.s_addr);
> So no IPv6 support here?
Correct. This is still PCUIF 9.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/20234
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I6529876a3c1116a79dd624312243d8ae48a41fe2
Gerrit-Change-Number: 20234
Gerrit-PatchSet: 10
Gerrit-Owner: lynxis lazus <lynxis at fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis at fe80.eu>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 01 Oct 2020 18:50:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: lynxis lazus <lynxis at fe80.eu>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201001/569f9bf2/attachment.htm>