Attention is currently required from: daniel, lynxis lazus.

pespin has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/38938?usp=email )

Change subject: gtp: add support for SGSN Context Req/Resp/Ack ......................................................................

Patch Set 18:

(15 comments)

File gtp/gtp.c:

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/3c656190_1c4b1b4e?usp... : PS18, Line 400: union gtp_packet *packet, int len, indentation

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/94d68fac_34b652cd?usp... : PS18, Line 888: const struct sockaddr_in *peer, union gtpie_member **ie, size_t ie_size) the "**ie" contents can be consitfied like you did in a previous patch right?

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/3300be40_0e35c5ef?usp... : PS18, Line 894: union gtpie_member resp_ie_elem[2] = {}; req_ie_elem?

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/d60eae7c_7036ac66?usp... : PS18, Line 925: union gtpie_member **ie, unsigned int ie_size) constify?

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/fbaefd92_d60ef6ab?usp... : PS18, Line 967: union gtpie_member *ie[GTP_MAX] = {}; why GTP_MAX elements if you are only configuring one below?

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/2a6a4083_fe8e046b?usp... : PS18, Line 981: union gtpie_member **ie, unsigned int ie_size) constify

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/0b94a0e4_a2f693e4?usp... : PS18, Line 1203: /* FIXME: parse PDP Address */ what about this? afaiu it's done bleow already?

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/8130bc5b_894a5337?usp... : PS18, Line 1217: /* FIXME: check for correct type and length */ what about this?

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/252ce551_c288d51d?usp... : PS18, Line 1429: /* FIXME: retransmission need to be implemented: afaiu this is already done by gtp_conf()?

File gtp/gtp_sgsn_ctx.h:

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/5c36c7a1_6a5b549c?usp... : PS18, Line 25: /* remote SGSN/MME request a Ctx from this peer */ adding a whitespace line before this one may help understand there are 2 blocks of states.

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/9497cc3c_be53a3c7?usp... : PS18, Line 36: SGSN_CTX_REQ_E_RX_REQ, I'd welcome if you can document here the type of value being passed as a parameter for each event expecting one. I find this incredibly useful when reading FSMs and making sure everything is fine.

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/5cbf9afc_2ba1519f?usp... : PS18, Line 52: struct llist_head local_reqs; we'll probably need some sort of hashtable here later on.

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/892085d1_7fa21a08?usp... : PS18, Line 67: struct osmo_fsm_inst *fsm; can we call this "fi" like virtually everywhere in our code base?

File gtp/gtp_sgsn_ctx.c:

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/950faa3f_268b016c?usp... : PS18, Line 325: llist_del(&req->list); req is not added to any llist during _alloc(), which means if user calls "req = sgsn_ctx_alloc(); sgsn_ctx_free(req);" weird stuff may happen?

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/9aed7755_f6110f53?usp... : PS18, Line 439: /* TODO: move tx GTPv1 into the fsm */ what about this and similar ones below?