Change in osmo-mgw[master]: mgcp_client: allow to reset endpoints on startup

dexter gerrit-no-reply at
Tue Jul 27 10:01:27 UTC 2021

dexter has posted comments on this change. ( )

Change subject: mgcp_client: allow to reset endpoints on startup

Patch Set 5:

File src/libosmo-mgcp-client/mgcp_client.c: 
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. 
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. 
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. 
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) 
File src/libosmo-mgcp-client/mgcp_client_vty.c: 
PS4, Line 161: eindpoint
> "endpoint" […]
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) 
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... […]
PS4, Line 202: startup
> ("on connect" as above) […]
PS4, Line 210: 			reset_ep_del = reset_ep;
> break; ?  or put the free() directly in the loop and return CMD_SUCCESS here?

To view, visit
To unsubscribe, or for help writing mail filters, visit

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I47e7ff858d5067b46d52329be5f362ff61c0dff8
Gerrit-Change-Number: 25008
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier at>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at>
Gerrit-Reviewer: pespin <pespin at>
Gerrit-CC: neels <nhofmeyr at>
Gerrit-Comment-Date: Tue, 27 Jul 2021 10:01:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr at>
Comment-In-Reply-To: pespin <pespin at>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list