Change in ...osmo-msc[master]: Implements a global switch on the network to disable call waiting.

[neels]

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/15120 )

Change subject: Implements a global switch on the network to disable call waiting.
......................................................................


Patch Set 1: Code-Review-1

(11 comments)

build failure: you need to adjust the vty transcript test that failed. You added the 'callwaiting' vty option, and the transcript test didn't expect that to appear.

https://gerrit.osmocom.org/#/c/15120/1//COMMIT_MSG 
Commit Message:

https://gerrit.osmocom.org/#/c/15120/1//COMMIT_MSG@22 
PS1, Line 22: of call waiting.
would be nice to briefly mention or reference issues of the unpredictable behavior


https://gerrit.osmocom.org/#/c/15120/1/include/osmocom/msc/gsm_data.h 
File include/osmocom/msc/gsm_data.h:

https://gerrit.osmocom.org/#/c/15120/1/include/osmocom/msc/gsm_data.h@258 
PS1, Line 258: 	uint8_t callwaiting;
rather use bool


https://gerrit.osmocom.org/#/c/15120/1/include/osmocom/msc/transaction.h 
File include/osmocom/msc/transaction.h:

https://gerrit.osmocom.org/#/c/15120/1/include/osmocom/msc/transaction.h@142 
PS1, Line 142: 					struct vlr_subscr *vsub);
Instead of adding this I would suggest the caller use

  trans_find_by_type(msc_a_for_vsub(vsub, true), TRANS_CC);

(to avoid API clutter)


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c 
File src/libmsc/gsm_04_08_cc.c:

https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1806 
PS1, Line 1806: 	struct gsm_trans *_trans = NULL;
> Not sure about this, but I got a segfault (in other code path) while testing, wasn't sure if it was  […]
as I read the code there shouldn't be a segfault, but it is better style to use a separate variable indeed.
Rather declare the separate variable at the start of the "if (!net->callwaiting)" scope because it isn't used elsewhere, and rather name it like existing_cc_trans or something to clarify.


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1879 
PS1, Line 1879: 		if (!net->callwaiting) {
(put the trans var decl here)


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1880 
PS1, Line 1880: 			_trans = trans_find_cc_by_vsub(net, vsub);
(use trans_find_by_type() here, see below)


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1886 
PS1, Line 1886: 					_trans->cc.state,
cosmetic: no need to log both the state number and the gsm48_cc_state_name(), just the name is enough


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1894 
PS1, Line 1894: 		if (!vsub->lu_complete) {
would make sense to keep this check before the callwaiting check. It's higher up the priority of reject reasons. so... (read below)


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1902 
PS1, Line 1902: 		}
<-- MARKER (see below)


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1917 
PS1, Line 1917: 		msc_a = msc_a_for_vsub(vsub, true);
and here is an msc_a already.

So I would suggest:

- move this msc_a = msc_a_for_vsub() up to "MARKER" above.
- put the net->callwaiting check right below that
- use trans_find_by_type(msc_a, TRANS_CC) instead of adding API


https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/msc_vty.c 
File src/libmsc/msc_vty.c:

https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/msc_vty.c@360 
PS1, Line 360: 		vty_out(vty, " callwaiting%s", VTY_NEWLINE);
(we often don't write back the default, so could only write 'no callwaiting' in case it is false)



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15120
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I3eb6f23f7103e3002874fb5d3a30c9de952202ae
Gerrit-Change-Number: 15120
Gerrit-PatchSet: 1
Gerrit-Owner: keith <keith at rhizomatica.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: keith <keith at rhizomatica.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Aug 2019 14:19:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: keith <keith at rhizomatica.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190808/46eac5db/attachment.htm>