This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
dexter gerrit-no-reply at lists.osmocom.orgdexter 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/d96ffb2e/attachment.htm>