Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12814 )
Change subject: VTY: add command show asciidoc ctrl ...................................................................... Patch Set 2: (3 comments) I agree with Vadim in that there are two separate features merged in one patch which should be separate: 1) exposing the list via CTRL, and 2) adding access modes. I would assume '1' is the less controversial change, and hence it should be first so it can be merged independently of the second. Whether or not we'd want to go this way with extneding the CTRL interface, and whether the proposed implementation complies with the CTRL interface in general is something I put into Daniels' hands, as he created CTRL originally. https://gerrit.osmocom.org/#/c/12814/2/include/osmocom/ctrl/control_cmd.h File include/osmocom/ctrl/control_cmd.h: https://gerrit.osmocom.org/#/c/12814/2/include/osmocom/ctrl/control_cmd.h@107 PS2, Line 107: enum ctrl_cmd_mode mode; > You're breaking ABI here. To give more context: As per our policy, this would require dding an entry to the TODO-RELEASE file, as it requires us to increment the LIBVERSION at the next version tag to avoid breaking backwards/forwards ABI compatibility. https://gerrit.osmocom.org/#/c/12814/2/include/osmocom/ctrl/control_cmd.h@152 PS2, Line 152: CTRL_CMD_DEFINE_STRUCT > Since this is a public header, we cannot modify existing symbols nor > definitions. this is required to ensure that old code can be built against new libraries. So you cannot change the number/order of arguments to any existing function or macro, but must always introduce a new one while keeping the old one as a compatibility wrapper with reasonable defaults. https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c File src/ctrl/control_vty.c: https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@89 PS2, Line 89: switch(cmd_el->mode){ > Missing space, should be 'switch ('. the philosophy of the Linux kernel coding style (which we use) is "if, switch, etc. are no functions, hence a space". -- To view, visit https://gerrit.osmocom.org/12814 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16f59bfca72a7dd9c268bc64499b26d82a2115d2 Gerrit-Change-Number: 12814 Gerrit-PatchSet: 2 Gerrit-Owner: Omar Ramadan <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-Comment-Date: Wed, 06 Feb 2019 08:21:41 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
