laforge at osmocom.org
Fri Sep 18 21:15:18 UTC 2020
Hi List and Lynxis,
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?
== 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.
== 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.
== 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.
== 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? I don't understand what benefit the extra 0/1
return value gives. The only other return value is -EINVAL if the **ptr
was NULL - an error situation that wouldn't exist if we simply returned
the pointer from the function.
- Harald Welte <laforge at osmocom.org> http://laforge.gnumonks.org/
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
More information about the osmocom-net-gprs