Change in osmo-mgw[master]: mgcp_vty: refactor endpoint number configuration

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 https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

neels gerrit-no-reply at lists.osmocom.org
Mon Jul 6 14:22:50 UTC 2020


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

Change subject: mgcp_vty: refactor endpoint number configuration
......................................................................


Patch Set 2: Code-Review-1

(9 comments)

https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/include/osmocom/mgcp/mgcp_trunk.h 
File include/osmocom/mgcp/mgcp_trunk.h:

https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/include/osmocom/mgcp/mgcp_trunk.h@51 
PS2, Line 51: 		} virtual;
better to use a different name, because 'virtual' is a C++ keyword... virt or virtual_trunk or ...
If only to avoid confusing code highlighting, like here in gerrit.


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c 
File src/libosmo-mgcp/mgcp_trunk.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@51 
PS2, Line 51: 	trunk->trunk_nr = nr;
(unrelated whitespace change)


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@87 
PS2, Line 87: 		offset_endpoints = 1;
(naming: first_endp_id or first_endp_nr? "offset" to me indicates that we are skipping some items in an array or data buffer, so for a couple minutes i was reading this code wrong. This variable only affects the endpoint id / number starting with at the start of the array, without an actual data offset. maybe it's just me though.)


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@99 
PS2, Line 99: 	 * however miss-initalation of the struct or memory corruption
'initialization'


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@103 
PS2, Line 103: 
Note that the vty allows vty_number_endpoints to be 0, in which case we probably should not invoke talloc?
Seems odd to allow zero endpoints, so see comment in mgcp_vty.c...


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@105 
PS2, Line 105: 	trunk->endpoints = _talloc_zero_array(trunk->cfg,
(separate issue: seems sketchy to use an API function that starts with an underscore and that isn't documented. Why not just talloc_zero_array(ctx, struct mgcp_endpoint*, number_endpoints) -- the only difference seems the name shown in the talloc report, right?)


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_vty.c 
File src/libosmo-mgcp/mgcp_vty.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_vty.c@713 
PS2, Line 713:       "number endpoints <0-65534>",
...this is where vty_number_endpoints == 0 is allowed by the vty.
I think the problem could be fixed trivially by a separate patch changing this to '<1-65534>'.
(Only if we're reasonably sure that no current config out there sanely uses 0 in the cfg file)


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_vty.c@936 
PS2, Line 936: 		vty_out(vty, "  timeslot first %u%s",
Preferably add new vty commands in a separate patch, because the commit log says "refactor"


https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/src/libosmo-mgcp/mgcp_vty.c@1179 
PS2, Line 1179:       "Timeslot options\n" "Number of E1 timeslots to use\n" "E1 timeslot count\n")
explain whether the used E1 timeslots end up being [first .. first+num] or [first .. num] ?



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

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I73b31e3c236a61ea0a6f76ef5ff98ce589f52c77
Gerrit-Change-Number: 19121
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 06 Jul 2020 14:22:50 +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/20200706/bf623842/attachment.htm>


More information about the gerrit-log mailing list