Change in libosmocore[master]: Gb: add a second NS implementation

historical

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/19417 )

Change subject: Gb: add a second NS implementation
......................................................................

Patch Set 24:

(23 comments)

Without knowing much about the context of this patch, I'd say it still needs a general review from its author to clean up stuff.

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/include/osmocom/gprs/gprs_ns2.h 
File include/osmocom/gprs/gprs_ns2.h:

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/include/osmocom/gprs/gprs_ns2.h@26 
PS24, Line 26: /* TODO: remove ! */
so we have a TODO: remove on a struct in a public header? That looks wrong ;)

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/include/osmocom/gprs/gprs_ns2.h@40 
PS24, Line 40: 	NS2_VC_MODE_BLOCKRESET, /* The will only use ALIVE/ACK no initiation */
"The VC will"
"on initiation"?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/include/osmocom/gprs/gprs_ns2.h@85 
PS24, Line 85: 			/* TODO: implement resource distribution */
So future ABI break?
Can we keep the structure private?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c 
File src/gb/gprs_ns2.c:

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@117 
PS24, Line 117: 		[NS_IE_IP_ADDR] = { TLV_TYPE_FIXED, 5 },
So parsing is going to fail for ipv6?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@217 
PS24, Line 217: 			snprintf(buf, buf_len, "udp)%s:%u<%u>%s:%u",
Ideally IPv6 should be enclosed in between breackets ([])

BTW, why is there no matching parenthesis, only ")" ?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@227 
PS24, Line 227: 		snprintf(buf, buf_len, "frgre)");
missing break?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@229 
PS24, Line 229: 		snprintf(buf, buf_len, "e1)");
missing break?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@248 
PS24, Line 248: 	char *buf = talloc_size(ctx, INET6_ADDRSTRLEN+10);
shouldn't this be INET6_ADDRSTRLEN*2 ? and +10 doesn't seem enough to me.

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@346 
PS24, Line 346: 	llist_add(&nsvc->list, &nse->nsvc);
Easiest here is to add to several lists at the end before returning, then you can drop 2 lines from "err:"

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@395 
PS24, Line 395: 	if (nsvc->priv && nsvc->bind->free_vc) {
uneeded {}

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@408 
PS24, Line 408: 		"GPRS/NS");
move to be in line with "(".

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@420 
PS24, Line 420:  * \param[out] result
this documenttion needs updating.

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@554 
PS24, Line 554:  * \param[out] reject A message filled to be sent back. Only used in failure cases.
docuemntation needs updating

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@675 
PS24, Line 675:  * \brief gprs_ns2_ip_connect
documentation needs updating.

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2.c@882 
PS24, Line 882: static bool gprs_fsm_vc_registered = false;
what about this?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_frgre.c 
File src/gb/gprs_ns2_frgre.c:

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_frgre.c@477 
PS24, Line 477: 	/* FIXME: actually send the data here instead of nsip_sendmsg() */
This is yet not implemented?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_frgre.c@524 
PS24, Line 524: 		rc = handle_nsfrgre_read(bfd);
Usual stuff: check if handle_nsfrgre_read() can cause the struct holding bfd to be freed. In those paths, return -EBADF and check that in next if here.

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_message.c 
File src/gb/gprs_ns2_message.c:

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_message.c@43 
PS24, Line 43: #define ERR_IF_NSVC_USES_SNS(nsvc, reason) 							\
?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_message.c@64 
PS24, Line 64: int gprs_ns2_validate_reset(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)
returning 1 or 0? this looks strange...

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_sns.c 
File src/gb/gprs_ns2_sns.c:

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_sns.c@268 
PS24, Line 268: 	remote.u.sin6.sin6_port = ip6->udp_port;
be careful, you may need htons() here

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_sns.c@301 
PS24, Line 301: 		remote.u.sin.sin_port = ip4->udp_port;
htons?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_sns.c@336 
PS24, Line 336: 		remote.u.sin6.sin6_port = ip6->udp_port;
htons?

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_vc_fsm.c 
File src/gb/gprs_ns2_vc_fsm.c:

https://gerrit.osmocom.org/c/libosmocore/+/19417/24/src/gb/gprs_ns2_vc_fsm.c@121 
PS24, Line 121: 	{ GPRS_NS2_EV_BLOCK,	"BLOCK" },
weird indentation

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

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I3525beef205588dfab9d3880a34115f1a2676e48
Gerrit-Change-Number: 19417
Gerrit-PatchSet: 24
Gerrit-Owner: lynxis lazus <lynxis at fe80.eu>
Gerrit-Assignee: daniel <daniel at totalueberwachung.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis at fe80.eu>
Gerrit-CC: daniel <daniel at totalueberwachung.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Tue, 08 Sep 2020 18:58:08 +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/20200908/751fda2f/attachment.htm>