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 <axilira...@gmail.com>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilira...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to