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

osmith gerrit-no-reply at lists.osmocom.org
Wed Aug 4 09:42:48 UTC 2021


osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/25120 )

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


Patch Set 5: Code-Review-1

(12 comments)

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?

https://gerrit.osmocom.org/c/osmo-mgw/+/25120/3/include/osmocom/mgcp_client/mgcp_client_pool_internal.h 
File include/osmocom/mgcp_client/mgcp_client_pool_internal.h:

https://gerrit.osmocom.org/c/osmo-mgw/+/25120/3/include/osmocom/mgcp_client/mgcp_client_pool_internal.h@21 
PS3, Line 21: thet
that


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/4/src/libosmo-mgcp-client/mgcp_client.c 
File src/libosmo-mgcp-client/mgcp_client.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/25120/4/src/libosmo-mgcp-client/mgcp_client.c@799 
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?


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c 
File src/libosmo-mgcp-client/mgcp_client_pool.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@43 
PS5, Line 43: clien
client


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@56 
PS5, Line 56: initalization
initialization


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@153 
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.


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@159 
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


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c 
File src/libosmo-mgcp-client/mgcp_client_vty.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@47 
PS5, Line 47: vty->index
just to be sure, is this intended? I'm wondering if vty->index has type
  struct mgcp_client_conf


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@50 
PS5, Line 50:  \
\ at the end of the macro looks wrong

(also, being curious: what's the advantage of using a macro here?)


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@329 
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?


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@394 
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?)


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@410 
PS5, Line 410: will drop ongoing calls
with the check below it won't drop ongoing calls


https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@531 
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 https://gerrit.osmocom.org/c/osmo-mgw/+/25120
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Icaaba0e470e916eefddfee750b83f5f65291a6b0
Gerrit-Change-Number: 25120
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
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: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210804/fe41736e/attachment.htm>


More information about the gerrit-log mailing list