Change in osmo-mgw[master]: libosmo-mgcp-client: extend the mgcp_client for MGW pooling

dexter gerrit-no-reply at
Wed Aug 4 15:04:05 UTC 2021

dexter has posted comments on this change. ( )

Change subject: libosmo-mgcp-client: extend the mgcp_client for MGW pooling

Patch Set 6:


Thanks for the review.

In order to get VTY tests I think we would need an application first. This could be a dummy application maybe or we could add some checks to osmo-bsc where the client is used. However the VTY for the MGCP Client is not very complex. At least for this patch I think I did not damage anything.

Also in the osmo-mgw we do not have any example configs for the client. In osmo-bsc I could add an example config but I don't know if this makes sense since I have already put an example into the manual. See also: osmo-bsc I8f33ab2cea04b545c403a6fe479aa963a0fc0d0d 
File include/osmocom/mgcp_client/mgcp_client_pool_internal.h: 
PS3, Line 21: thet
> that
File src/libosmo-mgcp-client/mgcp_client.c: 
PS4, Line 799: static int init_socket(struct mgcp_client *mgcp, unsigned int retry_n_ports)
> Adding the retry_n_ports and other slight tweaks to this function seem like a different logical chan […]
File src/libosmo-mgcp-client/mgcp_client_pool.c: 
PS5, Line 43: clien
> client
PS5, Line 56: initalization
> initialization
PS5, Line 153: put an MGCP client back into the pool
> maybe it's just me, but I find this a bit misleading: it sounds like the mgcp client was taken "out  […]
Yes it sounds a bit like that but I do not know how to explain it differently. I have added some more comments to make clear what actually happens - so nobody gets confused. 
PS5, Line 159: 	 *  not belong to a pool at all, the function call will have no effect. */
> this comment describes what the whole function does, so I'd merge it with the function description o […]
I think that is done automatically by doxygen. I have marked the comment with a /*! that should work. 
File src/libosmo-mgcp-client/mgcp_client_vty.c: 
PS5, Line 47: vty->index
> just to be sure, is this intended? I'm wondering if vty->index has type […]
It should be correct. Index is void pointer. See cfg_mgw_cmd, there I am assigning the configuration. 
PS5, Line 50:  \
> \ at the end of the macro looks wrong […]
I have to use this macro in each (common) VTY command to get the pointer to the conf. There are quite a few of them so it reduces some code dup. 
PS5, Line 329:  
> just to be sure: is this extra space on purpose? given that there's already two +1 above for indent_ […]
The extra space is on purpose. I use it to indent the mgw vty settings under the mgw x node by one space. 

The global_mgcp_client_indent_pool should be written only once. I don't really see what you mean but I managed to eliminate the global variable. 
PS5, Line 394: 			pool_member->nr, VTY_NEWLINE);
> (I guess for users it would be useful to have it block (not use it for new calls) and auto-remove th […]
I think it is better if blocking and removing is done consciously. It is also not done very often. 
PS5, Line 410: will drop ongoing calls
> with the check below it won't drop ongoing calls
PS5, Line 531: indent
> shouldn't this be talloc_strdup or something? but even better would be to get rid of the global var  […]
I would normally expect the indent string to be a constant, but who knows where an API user is getting this from.

I had a closer look to the global variables and I have now eliminated as much of them as I could. At the moment this VTY implementation can only manage one pool. This is a limitation, but at the Moment we do not have any application that would benefit from multiple pools. However, I wonder if it would be possible to manage multiple pools but the only way to do it is to use a vty->index that is populated by the VTY of the application. But think that would be too complicated for now.

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

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Icaaba0e470e916eefddfee750b83f5f65291a6b0
Gerrit-Change-Number: 25120
Gerrit-PatchSet: 6
Gerrit-Owner: dexter <pmaier at>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at>
Gerrit-Reviewer: neels <nhofmeyr at>
Gerrit-Reviewer: osmith <osmith at>
Gerrit-Reviewer: pespin <pespin at>
Gerrit-Comment-Date: Wed, 04 Aug 2021 15:04:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith <osmith at>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list