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

[Vadim Yanitskiy]

Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/12814 )

Change subject: VTY: add command show asciidoc ctrl
......................................................................


Patch Set 2: Code-Review-2

(12 comments)

https://gerrit.osmocom.org/#/c/12814/2//COMMIT_MSG
Commit Message:

https://gerrit.osmocom.org/#/c/12814/2//COMMIT_MSG@13
PS2, Line 13: CTRL command access mode indicator
It looks like you're submitting two squashed changes in a single one.
Why not to submit both separately?


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.


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.


https://gerrit.osmocom.org/#/c/12814/2/include/osmocom/ctrl/control_cmd.h@216
PS2, Line 216: CTRL_MODE_RW
Same comments apply here and below...


https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_cmd.c
File src/ctrl/control_cmd.c:

https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_cmd.c@61
PS2, Line 61: \a handle_ctrl_cmd
AFAIR, Neels recently figured out that manual referencing (\ref or \a) is not needed.


https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_cmd.c@74
PS2, Line 74: cmds_vec = vector_lookup_ensure(ctrl_node_vec, node
cosmetic: you could reduce nesting:

  cmds_vec = vector_lookup_ensure(ctrl_node_vec, node);
  if (!cmds_vec)
    return 0; // or -EFOOBAR?


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@83
PS2, Line 83: i=0
Missing spaces.


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 ('.


https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@90
PS2, Line 90: 	
Usually, we don't shift 'cases' from the 'switch' statement.


https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@100
PS2, Line 100: ""
"(unknown)"?


https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@104
PS2, Line 104: talloc_free
This allocation is only used at the beginning of the function, so it should be freed there.


https://gerrit.osmocom.org/#/c/12814/2/src/ctrl/control_vty.c@109
PS2, Line 109: DEFUN(show_ctrl_asciidoc_table,
             : 		show_ctrl_asciidoc_table_cmd,
             : "show asciidoc ctrl",
             : SHOW_STR "Asciidoc generation\n" "Generate table of all registered ctrl configurations\n")
             : {
This coding style itself deserves CR-2, sorry.



-- 
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: Jenkins Builder (1000002)
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 06:14:58 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190204/a6e48495/attachment.htm>