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.orgneels 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>