hi harald,
thanks for your review.
I've been reading through the ns2 code more thoroughly and had some thoughts for improvement. As we have no users yet, and the code is unreleased, we can still make changes now.
== unused argument in ns2_recv_vc
int ns2_recv_vc(struct gprs_ns2_inst *nsi, struct gprs_ns2_vc *nsvc, struct msgb *msg)
The 'nsi' is not used and should be redundant, as the nsvc has a back-pointer anyway, right?
Ack.
== consider using an osmo_ prefix to all symbols / types
The fact that the old code doesn't have that is a tribute to its age, and not something we need to keep. The current code has quite a bit of 'gprs_ns2' prefixing for types, but not for the symbols/functions. At least that inconsistency should be resolved, so all have the same prefix, even if it is without osmo.
The concept was prefix gprs_ns2 for public symbols and ns2 for internal since some functions are used across files within libosmocore. The osmo_ prefix sounds a lot cleaner.
== const-ify input arguments
If arguments pointing to data are used read-only, let's express that by the const qualifier. The easy ones are the likes of "gprs_ns2_is_frgre_bind(struct gprs_ns2_vc_bind *bind)", but there are definitely many more.
Ack.
== use of msgb->dst
In several other osmocom implementations we use msgb->dst to point to the underlying element receiving or transmitting a message. So I could imagine ns2_recv_vc() not only without the 'nsi' argument, but also without the 'nsvc' argument, if we introduce the convention that msgb->dst would always point to the ns-vc.
Not sure here, it has pros and cons. Just brainstorming.
Not sure if the effort for the msgb->dst is worth because we're only using it internal. I don't have an opinion on this. Except if we want to use the pointer for the resource distribution function [48.016 4.4a].
== output arguments vs. return value
There are functions like gprs_ns2_find_vc_by_sockaddr() where the result is not returned, but rather a **pointer output argument is used. What is the rationale here?
In general I like the difference between result and return code. ACK on gprs_ns2_find_vc_by_sockaddr() where there isn't much in the result code.
Best Regards, lynxis