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

dexter gerrit-no-reply at lists.osmocom.org
Wed Aug 4 15:04:05 UTC 2021


dexter 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 6:

(12 comments)

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

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
Done


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 chan […]
Done


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
Done


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


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  […]
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.


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 o […]
I think that is done automatically by doxygen. I have marked the comment with a /*! that should work.


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 […]
It should be correct. Index is void pointer. See cfg_mgw_cmd, there I am assigning the configuration.


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 […]
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.


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_ […]
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.


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 th […]
I think it is better if blocking and removing is done consciously. It is also not done very often.


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
Done


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  […]
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 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: 6
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 15:04:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith <osmith at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210804/36302eb4/attachment.htm>


More information about the gerrit-log mailing list