Attention is currently required from: neels, pespin.
7 comments:
File include/osmocom/pfcp/pfcp_endpoint.h:
Patch Set #3, Line 51: struct osmo_pfcp_endpoint {
Given this probably ends up as a public API in a shared library I think this is precisely the time t […]
not sure why endp is supposed to be better than endpoint? Does it matter?
Patch Set #3, Line 77: osmo_pfcp_endpoint_cb set_msg_ctx;
can we call all these callbacks with _cb at the end?
agreed
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.
does it matter in a prototype?
You call it API bloat, I call it do not break ABI next time you add something new in eg. […]
In general, the question is whether 'struct osmo_pfcp_endpoint' is supposed to be publicly exposed to every application. At this point you will have a very tight ABI coupling. The clean and safe approach is to go for what pespin is asking. In Osmocom we don't have a clear policy on what should be done. If we expect the struct will still be in flux a bit in the forseeable future, it might be an extra reason to go for the setter/getter functions.
File src/libosmo-pfcp/pfcp_endpoint.c:
Patch Set #3, Line 44: struct osmo_timer_list t1;
that's the point of the osmo_timer API: it keeps a list and waits for the first entry, later timers […]
Done
Patch Set #3, Line 177: static void pfcp_queue_timer_cb(void *data)
"timer_cb" is consistent: for example in osmo_fsm, the name is timer_cb, and it means that a timer h […]
Done
File src/libosmo-pfcp/pfcp_endpoint.c:
Patch Set #4, Line 268: /* Slight optimization: Add sent requests to the start of the list: we will usually receive a response shortly
You say to the start of the queue, but you do add_tail in both. […]
I haven't done a deep review, but if there is no reason to have a shared path/queue for requests and responses, I would also think it's more logical to keep them separate.
To view, visit change 28244. To unsubscribe, or for help writing mail filters, visit settings.