Attention is currently required from: daniel, lynxis lazus.

View Change

15 comments:

• File gtp/gtp.c:

  • Patch Set #18, Line 400: union gtp_packet *packet, int len,

    indentation

  • 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?

  • Patch Set #18, Line 894: union gtpie_member resp_ie_elem[2] = {};

    req_ie_elem?

  • Patch Set #18, Line 925: union gtpie_member **ie, unsigned int ie_size)

    constify?

  • Patch Set #18, Line 967: union gtpie_member *ie[GTP_MAX] = {};

    why GTP_MAX elements if you are only configuring one below?

  • Patch Set #18, Line 981: union gtpie_member **ie, unsigned int ie_size)

    constify

  • Patch Set #18, Line 1203: /* FIXME: parse PDP Address */

    what about this? afaiu it's done bleow already?

  • Patch Set #18, Line 1217: /* FIXME: check for correct type and length */

    what about this?

  • Patch Set #18, Line 1429: /* FIXME: retransmission need to be implemented:

    afaiu this is already done by gtp_conf()?

• 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.

  • Patch Set #18, 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.

  • Patch Set #18, Line 52: struct llist_head local_reqs;

    we'll probably need some sort of hashtable here later on.

  • Patch Set #18, 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:

  • 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(); sgsn_ctx_free(req);" weird stuff may happen?

  • Patch Set #18, Line 439: /* TODO: move tx GTPv1 into the fsm */

    what about this and similar ones below?

To view, visit change 38938. To unsubscribe, or for help writing mail filters, visit settings.