neels 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 4: (8 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. IIUC the list is only used once at program startup, just after reading in the cfg. Would it also work to just use conf->reset_epnames on connect? that way, a client that re-connects without restarting the program would also start using changes made to the list by telnet VTY immediately. 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 via mgcp_client_pending_add() even if response_cb is NULL, see (x) below. That way we can pass NULL to ignore the response and don't get an error log for it, and don't need to define an empty function. 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, and to not append a domain here. For example, I'm the admin of an osmo-mgw, and the domain configured in osmo-mgw.cfg was "@mgw", and say I'm now changing it to "@bsc". The next time the client restarts, I still want to make sure the legacy domain name also gets reset. So I might want to add both rtpbridge/*@mgw rtpbridge/*@bsc If you hardcode here that a domain is still added internally, you are taking this configuration freedom away from the user. So I think the solution is to simply not append a domain name, and document that the cfg must also contain the @domain part. If you think it is important to be able to omit the domain name in the config, then let's only add a domain name here when there is no '@' in the name yet? i.e. only add a domain when it is missing, so that the user can decide? 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. That's all that's needed, because we already avoid calling a NULL response_cb in mgcp_client_handle_response(). 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" Let's write "Add" instead of "Set", so it's clear that there can be more than one. Let's write "on connect" instead of "startup", because it's actually sent in mgcp_client_connect() 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) 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) Let's write "Remove an endpoint name from the reset-endpoint list" 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? -- 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: 4 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: Sat, 24 Jul 2021 23:06:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
