Change in osmo-mgw[master]: mgcp_client: add function to generate e1-endpoint names

[neels]

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

Change subject: mgcp_client: add function to generate e1-endpoint names
......................................................................


Patch Set 7:

(4 comments)

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

https://gerrit.osmocom.org/c/osmo-mgw/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@909 
PS7, Line 909: e1-0
the example says "e1-0" but trunk_id is <1-64>?


https://gerrit.osmocom.org/c/osmo-mgw/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@915 
PS7, Line 915: 	static const uint8_t valid_offsets[] = { 0, 0, 4, 0, 2, 4, 6, 0, 1, 2, 3, 4, 5, 6, 7 };
like last time, the const ones don't need to be static, or rather they should be further up in the .c file as global consts, and also it looks like there already are const arrays with the same numbers that this is code dup for


https://gerrit.osmocom.org/c/osmo-mgw/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@917 
PS7, Line 917: 	static char endpoint[MGCP_ENDPOINT_MAXLEN];
recently we're moving away from static char buffers and favor functions that write to a target buffer, or ones that use a talloc ctx to return talloc_asprinft() string.


https://gerrit.osmocom.org/c/osmo-mgw/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@931 
PS7, Line 931: 		return NULL;
it seems odd to do this validation in a function that provides a string from numbers.
Consider that you'd like to log an endpoint that is invalid, this function then won't produce a string for it.
It might be good to be able to output:

  Invalid endpoint: ds/e1-1/s-0/su99-17

Instead of

  Cannot compose MGCP e1-endpoint name, E1-timeslot number (0) is invalid!
  Invalid endpoint: NULL

or in other words only produce the "invalid" error logs at a point where we are taking in parameters from the user and explicitly validate them.

(now I see laforge said something similar in an earlier patch set)



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/19075
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Iec35b5bae8a7b07ddb3559f7114a24dcd10e8f14
Gerrit-Change-Number: 19075
Gerrit-PatchSet: 7
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: neels <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Thu, 09 Jul 2020 00:53:46 +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/20200709/a3ffadd6/attachment.htm>