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 <lyn...@fe80.eu> Gerrit-Assignee: daniel <dan...@totalueberwachung.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillm...@sysmocom.de> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: lynxis lazus <lyn...@fe80.eu> Gerrit-CC: daniel <dan...@totalueberwachung.de> Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Tue, 08 Sep 2020 18:58:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
