Attention is currently required from: daniel, laforge, pespin.
15 comments:
Commit Message:
Patch Set #18, Line 11: the API for the SGSN which uses libgtp as well as the external SGSN which communicates
cosmetic: too long commit log; your local git client should likely have warned you about it already?
No, my git client doesn't warn me about it. I'll check how to enable it.
File gtp/gtp.c:
Patch Set #18, Line 400: union gtp_packet *packet, int len,
indentation
Done
Patch Set #18, 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?
no. it gets modified.
Patch Set #18, Line 894: union gtpie_member resp_ie_elem[2] = {};
req_ie_elem?
Done
Patch Set #18, Line 925: union gtpie_member **ie, unsigned int ie_size)
constify?
no. it gets modified.
Patch Set #18, Line 967: union gtpie_member *ie[GTP_MAX] = {};
why GTP_MAX elements if you are only configuring one below?
Because of the GTP code base. I've removed ie_size/ie_len field from the outside api because the whole internal code expect to have an ie[GTPIE_SIZE].
I dislike this behavior, but don't see a good point in fixing this here.
Patch Set #18, Line 981: union gtpie_member **ie, unsigned int ie_size)
constify
I would like to allow the fsm to modify the list of ies on the way to the wire.
Or I have to copy the whole list.
Patch Set #18, Line 1203: /* FIXME: parse PDP Address */
what about this? afaiu it's done bleow already?
Done
Patch Set #18, Line 1217: /* FIXME: check for correct type and length */
what about this?
Done
Patch Set #18, Line 1429: /* FIXME: retransmission need to be implemented:
afaiu this is already done by gtp_conf()?
I need to write a test case for and check if gtp_conf() is really re-transmitting. The SGSN Req/Resp/Ack is a little bit special in this regard, because they are 3 packets instead of 2 (Req/Resp).
File gtp/gtp_sgsn_ctx.h:
Patch Set #18, 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.
Done
Patch Set #18, Line 52: struct llist_head local_reqs;
we'll probably need some sort of hashtable here later on.
Sure we could switch to a hash map. But there shouldn't be a huge list of outstanding SGSN contexts around.
Patch Set #18, Line 67: struct osmo_fsm_inst *fsm;
can we call this "fi" like virtually everywhere in our code base?
Done
File gtp/gtp_sgsn_ctx.c:
Patch Set #18, Line 325: llist_del(&req->list);
req is not added to any llist during _alloc(), which means if user calls "req = sgsn_ctx_alloc(); sg […]
It is added to a list by sgsn_ctx_req_alloc_outgoing or *sgsn_ctx_req_alloc_incoming.
sgsn_ctx_req_alloc() is never directly called except to one of those.
I'll add an underscope to the function to make it more clear.
Patch Set #18, Line 439: /* TODO: move tx GTPv1 into the fsm */
what about this and similar ones below?
I'm currently not very happy with this approach. The fsm doesn't transmit the packet,
it only validates a packet, but not transmitting it.
To view, visit change 38938. To unsubscribe, or for help writing mail filters, visit settings.