Alexander 'lynxis' Couzens
lynxis at fe80.eu
Mon Sep 21 15:50:17 UTC 2020
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?
> == 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.
> == 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
mail: lynxis at fe80.eu
jabber: lynxis at fe80.eu
gpg: 390D CF78 8BF9 AA50 4F8F F1E2 C29E 9DA6 A0DF 8604
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 833 bytes
Desc: OpenPGP digital signature
More information about the osmocom-net-gprs