Attention is currently required from: pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-upf/+/28244 )
Change subject: add pfcp_endpoint ...................................................................... Patch Set 3: (12 comments) File include/osmocom/pfcp/pfcp_endpoint.h: https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/93090be2_bb694c9f PS3, Line 46: * \param req If m is a PFCP Response to an earlier Request, req is that request message. */ > otherwise NULL? ACK https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/b26326e0_b8dbcd82 PS3, Line 51: struct osmo_pfcp_endpoint { > osmo_pfcp_endp seems like it's going to be a typing/reading change worth it ;) you mean the name should change to "osmo_pfcp_endp"? Can we focus on functional code review please? https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/8a4acdbf_c118bd79 PS3, Line 104: > In general I'm missing some APIs to set callbaks, local_node_id, etc. Do you mean add getters and setters? I'm not going to add API bloat for each struct member. File src/libosmo-pfcp/pfcp_endpoint.c: https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/acdc991e_d2e57507 PS3, 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. no, osmo_pfcp_msg has no pointer to an endpoint. Also it shouldn't. https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/5742e43e_b272eaa5 PS3, 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 wh […] that's the point of the osmo_timer API: it keeps a list and waits for the first entry, later timers untouched behind it in the list. (Recently I was surprised to see load caused by timers triggering every 10th of a second; then again that was multiplied by number of active lchans. PFCP messages are more like 5 seconds timeouts, and not firing continuously. I see no need to optimize here.) https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/56b3267e_a26eaee5 PS3, Line 73: int osmo_pfcp_queue_destructor(struct osmo_pfcp_queue_entry *qe) > why is the destructor not static? don't know why. it should be static, thanks. https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/3b53c02e_acea9681 PS3, Line 81: { .T = -19, .default_val = 15, .unit = OSMO_TDEF_S, > why some timers have names and some don't? there is no reason why. We use magic numbers for T = N almost everywhere in osmocom. But setting names made me notice a mistake, 27 should be 21, thx https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/c96a3198_e3edf700 PS3, 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 bi […] i'm pretty unsure about this timestamp coding, just know that wireshark ended up showing the expected date. do you have more specific code pointers? Or even, is there API I can call instead of re-implementing? OTOH the accuracy of the timestamp isn't really important AFAICT, the point of this is to have a unique value per application restart, for peers to detect that the peer has lost its state since last time. It's not critical to get this perfectly accurate, at least not until someone complains about it. https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/06ddc0a1_198c168d PS3, Line 139: unsigned int ep_t1(struct osmo_pfcp_endpoint *ep) > static thx https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/73368fa8_8cb857ce PS3, Line 144: unsigned int ep_keep_resp(struct osmo_pfcp_endpoint *ep) > static thx (I'm dreaming of a tool that tells us which unused #includes we have in .c and .h files, and which functions should be made static because used only in a single .c file... something like this should exist right? maybe the linter has such features??) https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/50760e6b_8c5b55fb PS3, Line 177: static void pfcp_queue_timer_cb(void *data) > Maybe rename to contain "expiry" in it? "timer_cb" is consistent: for example in osmo_fsm, the name is timer_cb, and it means that a timer has expired https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/7cb87127_da5b2dd2 PS3, Line 182: if (qe->m->is_response) { > I see you are basically putting together in the same list 2 different things: > […] Let me rephrase your comment: It would be more optimal to have separate lists for requests and responses. You're right, thanks -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/28244 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: Ic8d42e201b63064a71b40ca45a5a40e29941e8ac Gerrit-Change-Number: 28244 Gerrit-PatchSet: 3 Gerrit-Owner: neels <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Thu, 09 Jun 2022 21:46:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
