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

neels gerrit-no-reply at lists.osmocom.org
Sat Jul 24 23:06:41 UTC 2021


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 <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: Sat, 24 Jul 2021 23:06:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210724/ea4b7df0/attachment.htm>


More information about the gerrit-log mailing list