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

dexter gerrit-no-reply at lists.osmocom.org
Tue Jul 27 10:01:27 UTC 2021


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 <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: neels <nhofmeyr at 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 <nhofmeyr at sysmocom.de>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210727/0486fb86/attachment.htm>


More information about the gerrit-log mailing list