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

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

osmith gerrit-no-reply at
Wed Aug 4 09:42:48 UTC 2021

osmith has posted comments on this change. ( )

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

Patch Set 5: Code-Review-1


Very readable! Some thoughts:
* since this changes the VTY logic a lot, having VTY tests would be nice. from a quick glance it looks like there aren't any yet for mgcp-client, so not sure if this is reasonable.
* adjust / add an example config? 
File include/osmocom/mgcp_client/mgcp_client_pool_internal.h: 
PS3, Line 21: thet
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 change. Maybe put this into a separate commit? 
File src/libosmo-mgcp-client/mgcp_client_pool.c: 
PS5, Line 43: clien
PS5, Line 56: initalization
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 of the pool", as in, it can't be used anymore. but what happened earlier is, that it was picked and got a call assigned, the ref counter was increased. it will still be used if all other mgcp clients swimming in the pool have a higher refcount.

but that's more of a nitpick, all in all I find this patch and newly introduced code very readable. 
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 on top of the function and put it there 
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
  struct mgcp_client_conf 
PS5, Line 50:  \
\ at the end of the macro looks wrong

(also, being curious: what's the advantage of using a macro here?) 
PS5, Line 329:  
just to be sure: is this extra space on purpose? given that there's already two +1 above for indent_buf_len

the whole indent logic here seems a bit complicated TBH, is the global variable really necessary? it gets written to every time config_write_pool is called anyway.

after looking at this some more, it seems with each config_write_pool, the snprintf here tries to write more and more bytes after global_mgcp_client_indent_pool. that's certainly not what's intended? so if this variable is used, the passed size shouldn't be strlen + n, rather sizeof and a fixed buffer length? 
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 the mgw once all calls are served - maybe that's useful for a follow-up patch?) 
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 if possible IMHO, see my other comment

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: 5
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 09:42:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list