Change in libosmocore[master]: VTY: add command show asciidoc ctrl

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

Harald Welte gerrit-no-reply at lists.osmocom.org
Wed Feb 6 08:21:41 UTC 2019


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 <omar.ramadan93 at gmail.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Wed, 06 Feb 2019 08:21:41 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190206/b6751d1e/attachment.htm>


More information about the gerrit-log mailing list