Patch Set 1: Code-Review-1

(3 comments)

https://gerrit.osmocom.org/#/c/3187/1/include/osmocom/ranap/iu_client.h
File include/osmocom/ranap/iu_client.h:

Line 26:        /* TODO: It's not needed to store the full SCCP address for each
it would make sense to clean this up now, before we will first make this part 
of a public library while knowing that we will have to break ABI and API again 
to fix known issues.


https://gerrit.osmocom.org/#/c/3187/1/src/iu_client_vty.c
File src/iu_client_vty.c:

Line 35:       "logging asn1-debug (1|0)",
this is a view/enable node command, i.e. not configuration.  Any "logging" 
command with view/enable node by definition configures the logging to the 
current VTY session.  Do we have any code that actually ensures that the ASN1 
debug log ends up on that VTY (telnet session)?

Also, we are changing a global variable, so semantically it behaves completely 
different to other commands affecting logging to the current vty.

If it's not affecting the current VTY session, it should rather be a 
configuration statement and also saved to the config file.  Same is true for 
"logging asn1-xer-print" below.

Even then, existing logging configuration is per log target, and not global. I 
think this needs some more thought?


Line 80: DEFUN(cfg_iu_local_addr_pc, cfg_iu_local_addr_pc_cmd,
this should be integrated with the SCCP address book, so that a given Iu user 
can refer to a named SCCP address book entry to set the local point code.  
Let's at the very least add a TODO or FIXME comment here.


-- 
To view, visit https://gerrit.osmocom.org/3187
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a3f7ad15be03fb94689b4af6ccfa828c25f45c0
Gerrit-PatchSet: 1
Gerrit-Project: osmo-iuh
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: Yes

Reply via email to