dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/18372 )
Change subject: osmo-mgw: refactor endpoint and trunk handling ...................................................................... Patch Set 4: (24 comments) > Patch Set 4: > > Some of my questions/concerns were not answered yet. (forcing comments to be sent) can you check again? https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG@10 PS2, Line 10: implemented in various placed (mostly mgcp_protocol.c). Also we use > places Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG@14 PS2, Line 14: The trunk and endpoint handling in osmo-mgw is still very complex and > This paragraph is repeated. Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG@25 PS2, Line 25: - rename struct mgcp_trunk_config to mgcp_trunk and "tcfg" to "trunk" > Having an "mgcp_trunk" structure and a "trunk" one looks confusing to me. the one is the struct name and the other is the symbol name used in the code. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3//COMMIT_MSG@30 PS3, Line 30: - get rid of deprecated trunk parameters (leftorvers from the > leftovers Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/include/osmocom/mgcp/mgcp_endp.h File include/osmocom/mgcp/mgcp_endp.h: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/include/osmocom/mgcp/mgcp_endp.h@81 PS2, Line 81: /*! Backpointer to the related trunk */ > "Backpointer to the trunk this endpoint belongs to" would probably be more > clear for newcomers to un […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_codec.c File src/libosmo-mgcp/mgcp_codec.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_codec.c@289 PS3, Line 289: /* FIXME: implement meaningful checks to make sure that the given codec > AFAIU we are adding a regression here by dropping this code? I am very sure it won't introduce a regression. I have double checked it now. In my opinion the code is even introducing major problems. First of all the removal of the code does not change the behavior of osmo-mgw, the reason for this is because no_audio_transcoding is initalized to 0 and we usually do not switch it on, otherwise we would experience a lot of problems! So only if no_audio_transcoding is 1 the MGW would check the codec name against a pre-configured constant (vty) and if the requested codec does not match that constant CRCX and MDCX would fail. Those VTY settings were there from the beginning and I do not know why I tried keep them but in my opinion this is a very wired way to deal with the lack of transcoding. Apart from that the relevant VTY commands are already marked as deprecated. Given that making the is_codec_compatible() check take effect is switched off when we use osmo-mgw I think there is no regression possible. We might even think to remove no_audio_transcoding as well. I am not done yet but I think there are a couple more strange trunk properties that do not fit in our new model. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c File src/libosmo-mgcp/mgcp_endp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@101 PS2, Line 101: (epname, MGCP_ENDPOINT_PREFIX_VIRTUAL_TRUNK, > what if len(epname) < prefix_len? Is strncmp safe here? Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@158 PS2, Line 158: * wildarded endpoint searches that picks the next free endpoint on > wildcarded Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@183 PS2, Line 183: endp = trunk->endpoints[i]; > (We should move to a hashtable with key=str and val=ptr at some point now > that we use strings. […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@188 PS2, Line 188: "(trunk:%i) found endpoint: %s\n", > %d Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@196 PS2, Line 196: "(trunk:%i) Not able to find specified endpoint: %s\n", > %d Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@254 PS2, Line 254: if (trunk->trunk_type == MGCP_TRUNK_VIRTUAL) { > (At some point we should have function pointers to do type-specific stuff. […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_endp.c File src/libosmo-mgcp/mgcp_endp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@100 PS3, Line 100: if (strlen(epname) <= prefix_len) > So I was asking about strncmp because I don't know if the 2 strings here are > null terminated. […] They are null terminated, but I think there is still a problem. strncmp compares UP TO n char s. If the epname is lets say only 3 chars long and by chance these chars match the first 3 chars of the trunk prefix we would get a match too. But now I check the length, so it should be fine. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@109 PS3, Line 109: return epname; > trailing whitespace Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@216 PS3, Line 216: LOGP(DLMGCP, LOGL_ERROR, "missing domain name in endpoint name \"%s\", expecting '%s'\n", > SO you first use double quote for epname, but single quote for cfg->domain? Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@226 PS3, Line 226: LOGP(DLMGCP, LOGL_ERROR, "wrong domain name in endpoint name \"%s\", expecting '%s'\n", > Same Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@285 PS2, Line 285: /* FIXME: We hardcode to pick cfg->virt_trunkl, but > trailing whitespace Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@288 PS2, Line 288: struct rate_ctr_group *rate_ctrs = trunk->mgcp_general_ctr_group; > The rate_ctrs used below look general and not related to a trunk, so they > should mbe moved to be und […] I think I understand the problem now. The counters were allocated per trunk before. I did not change it, but when I look at the counters, it really looks like if the intension was to create some per-mgw health monitoring. I have now splatted that so that the counters are separate and global in the cfg. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@996 PS2, Line 996: struct mgcp_endpoint *endp = p->endp; > Swap these two lines: […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@1220 PS2, Line 1220: struct mgcp_endpoint *endp = p->endp; > Same, swap these two lines. Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_sdp.c File src/libosmo-mgcp/mgcp_sdp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_sdp.c@386 PS2, Line 386: LOGP(DLMGCP, LOGL_NOTICE, "endpoint:%s, failed to add codec\n", endp->name); > I see a lot of endpoint:%s logging, what about introducing LOGPENDP(endp, > CAT, "... […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_trunk.c File src/libosmo-mgcp/mgcp_trunk.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_trunk.c@30 PS2, Line 30: static const struct rate_ctr_desc mgcp_general_ctr_desc[] = { > As I said, to me lots of these counters are not per-trunk, but global, so > they shouldn't be here. Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/tests/mgcp/mgcp_test.c File tests/mgcp/mgcp_test.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/tests/mgcp/mgcp_test.c@74 PS2, Line 74: #define AUEP1_RET "500 158663169 FAIL\r\n" > What about these changes in behavior? expected? I don't recall seeing any > comment in commit deescrip […] The problem is that there are still leftovers of an E1 trunk implementation in osmo-mgw. This implementation was never finished. This tests relates to an endpoint of the old E1 implementation. So since E1 is not working and did never work we now reject anything that goes to an E1 trunk. This is why the AUEP here now fails, which is correct. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/tests/mgcp/mgcp_test.c File tests/mgcp/mgcp_test.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/tests/mgcp/mgcp_test.c@1461 PS3, Line 1461: /* Allocate 5@mgw and let osmo-mgw pick a codec from the list */ > I'm still wondering why are you changing the behavior of the test. […] I have checked it now back. (See also comment in mgcp_codec.c) See also CRCX_MULT_GSM_EXACT, there are a lot of codecs configured. Right at the front is one that has payload type 0 assigned to it. I have removed the possibility to "force" a specific codec type when no transcoding is available (which is a wired way to deal with transcoding). So now can no longer set an audio_name, so the code will pick the first codec in the list, which has payload_type 0, so as far as the test is concerned it does what is expected. -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/18372 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ice8aaf03faa2fd99074f8665eea3a696d30c5eb3 Gerrit-Change-Number: 18372 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pma...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Fri, 29 May 2020 11:25:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
