21 Sep
2020
21 Sep
'20
5:50 p.m.
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
--
Alexander Couzens
mail: lynxis(a)fe80.eu
jabber: lynxis(a)fe80.eu
gpg: 390D CF78 8BF9 AA50 4F8F F1E2 C29E 9DA6 A0DF 8604