Attention is currently required from: neels.

View Change

14 comments:

• File include/osmocom/pfcp/pfcp_endpoint.h:

  • Patch Set #3, Line 46: * \param req If m is a PFCP Response to an earlier Request, req is that request message. */

    otherwise NULL?

  • Patch Set #3, Line 51: struct osmo_pfcp_endpoint {

    osmo_pfcp_endp seems like it's going to be a typing/reading change worth it ;)

  • Patch Set #3, Line 77: osmo_pfcp_endpoint_cb set_msg_ctx;

    can we call all these callbacks with _cb at the end?

  • Patch Set #3, Line 99: int osmo_pfcp_endpoint_tx(struct osmo_pfcp_endpoint *ep, struct osmo_pfcp_msg *m);

    something more meaningful like "pfcp_msg" instead of "m" may be worth a change.

  • Patch Set #3, Line 104:

    In general I'm missing some APIs to set callbaks, local_node_id, etc.

• File src/libosmo-pfcp/pfcp_endpoint.c:

  • Patch Set #3, Line 42: struct osmo_pfcp_msg *m;

    Doesn't m already have a backpointer to ep? May make more sense to just have that one.

  • Patch Set #3, Line 44: struct osmo_timer_list t1;

    This can be probably optimized to be only 1 timer per list, by moving them to the end of the list when transmitting.

  • Patch Set #3, Line 73: int osmo_pfcp_queue_destructor(struct osmo_pfcp_queue_entry *qe)

    why is the destructor not static?

  • Patch Set #3, Line 81: { .T = -19, .default_val = 15, .unit = OSMO_TDEF_S,

    why some timers have names and some don't?

  • Patch Set #3, Line 122: /* time() returns seconds since 1970 (UNIX epoch), but the recovery_time_stamp is coded in the NTP format, which is

    you can check open5gs code, iirc some stuff exists to handle ntp timestamps and I also added some bits for Gy interface.

  • Patch Set #3, Line 139: unsigned int ep_t1(struct osmo_pfcp_endpoint *ep)

    static

  • Patch Set #3, Line 144: unsigned int ep_keep_resp(struct osmo_pfcp_endpoint *ep)

    static

  • Patch Set #3, Line 177: static void pfcp_queue_timer_cb(void *data)

    Maybe rename to contain "expiry" in it?

  • Patch Set #3, Line 182: if (qe->m->is_response) {

    I see you are basically putting together in the same list 2 different things:

    • List of transmitted request messages (in order to retransmit them at interval times).
    • List of transmitted response messages (no retrans happening, only keeping them in order to match and resend when a retransmit req arrives).

    I don't see the point in having them in the same list, because they are looked up and used at different scenarios. Having it in different lists means you don't need to loop over all requests+reposnses every time, only about the important subset (req or resp). See for instance how libgtp does it, since I believe it's the same system as in GTP.

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