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.
Regards, Harald
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
On Mon, Sep 21, 2020 at 05:50:17PM +0200, Alexander 'lynxis' Couzens wrote:
== 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.
oh, great. I somehow missed that.
The osmo_ prefix sounds a lot cleaner.
I guess we can keep it as-is. If you want, go for osmo_ns2 as public prefix, but I'm not sure it's worth the effort at this point.
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.
yes, let's just keep it as-is.
Except if we want to use the pointer for the resource distribution function [48.016 4.4a].
interesting. I'm not deep enough into the topic to have an opinion about it right now, sorry.
There are functions like gprs_ns2_find_vc_by_sockaddr() where the result is not returned, but rather a **pointer output argument is
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.
agreed. return the result whenever there is nothing to return.
Thanks.
osmocom-net-gprs@lists.osmocom.org
-
Alexander 'lynxis' Couzens -
Harald Welte -
Harald Welte