dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/25008 )
Change subject: mgcp_client: allow to reset endpoints on startup ...................................................................... Patch Set 5: (9 comments) https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c File src/libosmo-mgcp-client/mgcp_client.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@793 PS4, Line 793: llist_add_tail(&actual_reset_ep->list, &mgcp->actual.reset_epnames); > seems to me that it is not necessary to copy this list. […] The list is copied because osmo-mgcp-client is designed that way. The vty configuration is separated from the client itsself. When the client is started it copies all configuration items. Doing it differently would break this pattern. We also cannot just copy the llist head. I think it is better to keep it this way. https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@854 PS4, Line 854: void _ignore_mgcp_response(struct mgcp_response *response, void *priv) { } > hm, interesting! I think we should change mgcp_client_tx() to still store an > entry […] I have checked this back now. Unfortunately we cannot change this because there is a bug that whould lead into a memory leak, see change id: I83938ff47fa8570b8d9dc810a184864a0c0b58aa I don't really know how to fix this, but probably we would need some garbage collector that deletes entries from the queue when there is no response from some time. I think this problem is not only triggered by mgcp_client_fsm, but may also easily occur when someone is using the bare mgcp-client without its fsm extensions. This would require a separate patch/ticket. My idea would be just having a timestamp in each queue entry and when there is no response after a minute or so it would delete it from the queue. For now we could remove the dummy function and live with the error message or just keep it the way it ist. It does not hurt. https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@905 PS4, Line 905: epname = _mgcp_client_name_append_domain(mgcp, reset_ep->name); > I think it is more elegant to define the complete endpoint name in the > config, […] This is intentional. There is already a way to specify the domain name in the config. If you would allow to specify the domain name again in this reset endpoint feature than you might get an invalid configuration. The configuration would also be redundant since the domain name is already specified at a different point in the config. It makes no sense to configure a reset endpoint rtpbridge/*@bsc when the domain name is configured as @mgw. The MGW would reject rtpbridge/*@bsc because it would expect the domain name to be @mgw. https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@1052 PS4, Line 1052: if (response_cb != NULL) { > (x) mentioned above, here remove the response_cb != NULL condition. […] (see above, this works around a memory leak) https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c File src/libosmo-mgcp-client/mgcp_client_vty.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@161 PS4, Line 161: eindpoint > "endpoint" […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@175 PS4, Line 175: /* the domain name is not part of the actual endpoint name */ > (but i think it should be) (I see that differently, see my other comment) https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@188 PS4, Line 188: if (osmo_strlcpy(reset_ep->name, argv[0], sizeof(reset_ep->name)) >= sizeof(reset_ep->name)) { > repeated osmo_strlcpy with line above... […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@202 PS4, Line 202: startup > ("on connect" as above) […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@210 PS4, Line 210: reset_ep_del = reset_ep; > break; ? or put the free() directly in the loop and return CMD_SUCCESS here? Done -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/25008 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I47e7ff858d5067b46d52329be5f362ff61c0dff8 Gerrit-Change-Number: 25008 Gerrit-PatchSet: 5 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: neels <nhofm...@sysmocom.de> Gerrit-Comment-Date: Tue, 27 Jul 2021 10:01:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofm...@sysmocom.de> Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
