This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Patch Set 1:
(7 comments)
I guess if you agree with harald's comment, then most of this will go away anyway, but here goes:
https://gerrit.osmocom.org/#/c/7677/1/include/osmocom/msc/ussd.h
File include/osmocom/msc/ussd.h:
Line 12: /* Forward declarations to avoid mutual include */
(I'd drop the comment, we do this all the time, so everyone knows why anyway)
https://gerrit.osmocom.org/#/c/7677/1/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:
Line 1384: static struct cmd_node ussd_node = {
How much USSD config will follow? Does it really warrant a new sub-node?
Line 1414: else {
the vty does pretty tight input checking, you will *never* get any other string than exactly "none" or "local" here. No need for the else.
Line 1429: vty_out(vty, "none%s", VTY_NEWLINE);
(in case you remove the sub-node, you could write nothing if "none" is the default.)
https://gerrit.osmocom.org/#/c/7677/1/src/libmsc/ussd.c
File src/libmsc/ussd.c:
Line 41: const struct ss_request *req) = NULL;
If external handlers will show up in the future, they need to be able to provide their handler function, so this function type should be a typedef in a .h file. However, the choice of enum values to configure which ussd handler is active looks like there will be no external handler in a modular "function-pointery" way. It feels a bit overkill to have both enum and function pointer: either have a publicly configurable function pointer, or an enum value and switch() on that value when handling ussd.
Line 142: {
If there's going to be a shutdown and a choice of multiple handlers, there probably need to be shutdown functions per handler? So instead of just one handler function, you'd need a struct of function pointers, a bit like struct vlr_ops.
https://gerrit.osmocom.org/#/c/7677/1/src/osmo-msc/msc_main.c
File src/osmo-msc/msc_main.c:
Line 634: /* Initialize USSD */
("init_ussd" pretty much says it all, right?)
--
To view, visit https://gerrit.osmocom.org/7677
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0b3c5ccea4054113e8e23109b1ab68d9f0e18497
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-HasComments: Yes