[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-22 Thread neels 
neels has submitted this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..

tweak lots of logging

Make lots of small tweaks in logging around RUA to SCCP context maps, CN
link selection and HNBAP:

- remove duplicate log context (e.g. LOGHNB() already shows the HNB Id)
- bring more sense into logging categories and levels / reduce noise
- add and clarify the details being logged at what points in time

Related: SYS#6412
Change-Id: I41275d8c3e272177976a9302795884666c35cd06
---
M include/osmocom/hnbgw/context_map.h
M src/osmo-hnbgw/context_map.c
M src/osmo-hnbgw/context_map_sccp.c
M src/osmo-hnbgw/hnbgw.c
M src/osmo-hnbgw/hnbgw_cn.c
M src/osmo-hnbgw/hnbgw_hnbap.c
M src/osmo-hnbgw/hnbgw_l3.c
M src/osmo-hnbgw/hnbgw_rua.c
8 files changed, 94 insertions(+), 41 deletions(-)

Approvals:
  Jenkins Builder: Verified
  laforge: Looks good to me, but someone else must approve
  neels: Looks good to me, approved




diff --git a/include/osmocom/hnbgw/context_map.h 
b/include/osmocom/hnbgw/context_map.h
index 7017491..f7335a7 100644
--- a/include/osmocom/hnbgw/context_map.h
+++ b/include/osmocom/hnbgw/context_map.h
@@ -7,8 +7,9 @@

 #define LOG_MAP(HNB_CTX_MAP, SUBSYS, LEVEL, FMT, ARGS...) \
LOGHNB((HNB_CTX_MAP) ? (HNB_CTX_MAP)->hnb_ctx : NULL, \
-  SUBSYS, LEVEL, "RUA-%u %s MI=%s%s%s: " FMT, \
+  SUBSYS, LEVEL, "RUA-%u SCCP-%u %s MI=%s%s%s: " FMT, \
   (HNB_CTX_MAP) ? (HNB_CTX_MAP)->rua_ctx_id : 0, \
+  (HNB_CTX_MAP) ? (HNB_CTX_MAP)->scu_conn_id : 0, \
   (HNB_CTX_MAP) ? \
((HNB_CTX_MAP)->cnlink ? (HNB_CTX_MAP)->cnlink->name \
  : ((HNB_CTX_MAP)->is_ps ? "PS" : "CS")) \
diff --git a/src/osmo-hnbgw/context_map.c b/src/osmo-hnbgw/context_map.c
index 4d46b11..d0e0d80 100644
--- a/src/osmo-hnbgw/context_map.c
+++ b/src/osmo-hnbgw/context_map.c
@@ -26,6 +26,8 @@

 #include 

+#include 
+
 #include 

 #include 
@@ -92,7 +94,7 @@

llist_add_tail(&map->hnb_list, &hnb->map_list);

-   LOG_MAP(map, DRUA, LOGL_INFO, "New RUA CTX\n");
+   LOG_MAP(map, DRUA, LOGL_DEBUG, "New RUA CTX\n");
return map;
 }

@@ -123,8 +125,17 @@

hash_add(hsu->hnbgw_context_map_by_conn_id, 
&map->hnbgw_sccp_user_entry, new_scu_conn_id);

-   LOG_MAP(map, DMAIN, LOGL_INFO, "Creating new Mapping RUA CTX %u <-> SCU 
Conn ID %u to %s on %s\n",
-   map->rua_ctx_id, new_scu_conn_id, cnlink_selected->name, 
hsu->name);
+   LOGP(DRUA, LOGL_NOTICE, "New conn: %s '%s' RUA-%u <-> SCCP-%u %s%s%s %s 
l=%s<->r=%s\n",
+osmo_sock_get_name2_c(OTC_SELECT, 
osmo_stream_srv_get_ofd(map->hnb_ctx->conn)->fd),
+hnb_context_name(map->hnb_ctx), map->rua_ctx_id,
+new_scu_conn_id,
+cnlink_selected->name,
+cnlink_selected->use.remote_addr_name ? " " : "",
+cnlink_selected->use.remote_addr_name ? : "",
+hsu->name,
+osmo_ss7_pointcode_print(hsu->ss7, hsu->local_addr.pc),
+osmo_ss7_pointcode_print2(hsu->ss7, 
cnlink_selected->remote_addr.pc)
+   );

return 0;
 }
diff --git a/src/osmo-hnbgw/context_map_sccp.c 
b/src/osmo-hnbgw/context_map_sccp.c
index 3bdec8c..1f2cd09 100644
--- a/src/osmo-hnbgw/context_map_sccp.c
+++ b/src/osmo-hnbgw/context_map_sccp.c
@@ -81,7 +81,8 @@
struct osmo_fsm_inst *fi = osmo_fsm_inst_alloc(&map_sccp_fsm, map, map, 
LOGL_DEBUG, NULL);
OSMO_ASSERT(fi);
osmo_fsm_inst_update_id_f_sanitize(fi, '-', "%s-%s-SCCP-%u", 
hnb_context_name(map->hnb_ctx),
-  map->is_ps ? "PS" : "CS", 
map->scu_conn_id);
+  map->cnlink ? map->cnlink->name : 
(map->is_ps ? "PS" : "CS"),
+  map->scu_conn_id);

OSMO_ASSERT(map->sccp_fi == NULL);
map->sccp_fi = fi;
diff --git a/src/osmo-hnbgw/hnbgw.c b/src/osmo-hnbgw/hnbgw.c
index 571ff16..fc9e698 100644
--- a/src/osmo-hnbgw/hnbgw.c
+++ b/src/osmo-hnbgw/hnbgw.c
@@ -350,13 +350,29 @@

 const char *hnb_context_name(struct hnb_context *ctx)
 {
+   char *result;
if (!ctx)
return "NULL";

+   if (ctx->conn) {
+   char hostbuf_r[INET6_ADDRSTRLEN];
+   char portbuf_r[6];
+   int fd = osmo_stream_srv_get_ofd(ctx->conn)->fd;
+
+   /* get remote addr */
+   if (osmo_sock_get_ip_and_port(fd, hostbuf_r, sizeof(hostbuf_r), 
portbuf_r, sizeof(portbuf_r), false) == 0)
+   result = talloc_asprintf(OTC_SELECT, "%s:%s", 
hostbuf_r, portbuf_r);
+   else
+   result = "?";
+   } else {
+   result = "disconnected";
+   }
+
if (g_hnbgw->config.log_prefix_hnb_id)
-   return ctx->identity_info;
+   result = tal

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-20 Thread neels 
Attention is currently required from: laforge, pespin.

neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 7: Code-Review+2


--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: neels 
Gerrit-Reviewer: pespin 
Gerrit-Attention: laforge 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Wed, 21 Jun 2023 02:24:41 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-20 Thread neels 
Attention is currently required from: laforge, pespin.

neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 7:

(2 comments)

File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/c39b412c_9febad10
PS3, Line 515:  LOG_CNLINK(cnlink, DCN, LOGL_NOTICE, "using: cs7-%u %s <-> %s 
%s %s\n",
> #define CNLINK_PRINT_FMT "cs7-%u %s <-> %s %s %s" […]
i don't see any improvement, sorry


File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/b5200691_b14824d1
PS7, Line 635:  hnbgw_cnlink_log_self(cnlink);
> LOGP(DCN, LOGL_NOTICE, "Using " CNLINK_PRINT_FMT "\n",  
> CNLINK_PRINT_ARGS(cnlink)); […]
I could have the identical 5-line LOG_CNLINK() statement twice.
Instead I chose to have a static function and not dup those 5 lines.
IMHO this is not important.



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: pespin 
Gerrit-Attention: laforge 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Wed, 21 Jun 2023 02:23:54 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels 
Comment-In-Reply-To: laforge 
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-16 Thread laforge 
Attention is currently required from: neels.

laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 7: Code-Review+1


--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: pespin 
Gerrit-Attention: neels 
Gerrit-Comment-Date: Sat, 17 Jun 2023 06:16:34 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-16 Thread pespin 
Attention is currently required from: neels, laforge.

pespin has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 7:

(2 comments)

File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/3543a991_7b1dee0e
PS3, Line 515:  LOG_CNLINK(cnlink, DCN, LOGL_NOTICE, "using: cs7-%u %s <-> %s 
%s %s\n",
> I am not using multi line logging here ... ? […]
#define CNLINK_PRINT_FMT "cs7-%u %s <-> %s %s %s"
#define CNLINK_PRINT_ARGS(cnlink) ss7->cfg.id, \
   osmo_ss7_pointcode_print(ss7, 
cnlink->hnbgw_sccp_user->local_addr.pc), \
   osmo_ss7_pointcode_print2(ss7, cnlink->remote_addr.pc), \
   cnlink->name, cnlink->use.remote_addr_name ? : "(default 
remote point-code)"


File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/3b6a289c_b8e52303
PS7, Line 635:  hnbgw_cnlink_log_self(cnlink);
LOGP(DCN, LOGL_NOTICE, "Using " CNLINK_PRINT_FMT "\n",  
CNLINK_PRINT_ARGS(cnlink));

(or simply copy the entire log line if you are really expecting to use it 
twice).

Also, I'm note sure why cannot LOG_CNLINK or alike be used there instead. Maybe 
LOG_CNLINK should be extended with more info?



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: neels 
Gerrit-Attention: laforge 
Gerrit-Comment-Date: Fri, 16 Jun 2023 10:08:27 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels 
Comment-In-Reply-To: laforge 
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-15 Thread neels 
Attention is currently required from: laforge, pespin.

neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 7:

(1 comment)

File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/2734f3ed_06f27d8a
PS3, Line 515:  LOG_CNLINK(cnlink, DCN, LOGL_NOTICE, "using: cs7-%u %s <-> %s 
%s %s\n",
> multi-line logging is allways a PITA  from the point of anyone trying to 
> process log files. […]
I am not using multi line logging here ... ?
(and i am very aware that they must not be used)

I initially had no idea what Pau meant by the comment:
"This would rpboably better with a couple defines _FMT and _ARGS, so that it 
ends up in same log line and not in an extra one."

In the meantime, it has become clear that Pau means the LOG_CNLINK(LOGL_DEBUG) 
in line 581, followed by calling hnbgw_cnlink_log_self() which will 
LOG_CNLINK(LOGL_NOTICE) right after that.

I've explained that there is a minor detail being logged on DEBUG, followed by 
a very pivotal log line on NOTICE. These log lines are separate on purpose, and 
they should both be there; by my explicit choice made from reading log output 
and making sure that all the interesting bits show, while still ensuring a 
useful log on more quiet log levels. The DEBUG log is part of a family of log 
lines aimed at "tracing" all the ss7 and sccp instance decisions, it should not 
clutter the NOTICE log. The NOTICE log is a marker showing at which point in 
time the cnlink is fully configured for the first time.

I am still not clear what Pau means by "with a couple defines _FMT and _ARGS", 
I guessed the source file and line info. If yes, my response is that this log 
line happens only at program startup and happens exactly once per cnlink, so 
carrying the caller's source file+line info is not important. It is aimed at 
telling the user the resolved cnlink config; the DEBUG log is aimed at 
indicating which code path chose that config and why.

I have specifically made these choices on purpose, and it seems there is a 
misunderstanding in the code review?



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: laforge 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Fri, 16 Jun 2023 02:54:28 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels 
Comment-In-Reply-To: laforge 
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-15 Thread neels 
Attention is currently required from: neels, pespin.

Hello Jenkins Builder, pespin,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171

to look at the new patch set (#7).

Change subject: tweak lots of logging
..

tweak lots of logging

Make lots of small tweaks in logging around RUA to SCCP context maps, CN
link selection and HNBAP:

- remove duplicate log context (e.g. LOGHNB() already shows the HNB Id)
- bring more sense into logging categories and levels / reduce noise
- add and clarify the details being logged at what points in time

Related: SYS#6412
Change-Id: I41275d8c3e272177976a9302795884666c35cd06
---
M include/osmocom/hnbgw/context_map.h
M src/osmo-hnbgw/context_map.c
M src/osmo-hnbgw/context_map_sccp.c
M src/osmo-hnbgw/hnbgw.c
M src/osmo-hnbgw/hnbgw_cn.c
M src/osmo-hnbgw/hnbgw_hnbap.c
M src/osmo-hnbgw/hnbgw_l3.c
M src/osmo-hnbgw/hnbgw_rua.c
8 files changed, 94 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/71/33171/7
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: neels 
Gerrit-Attention: pespin 
Gerrit-MessageType: newpatchset

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-15 Thread laforge 
Attention is currently required from: neels, pespin.

laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 5:

(1 comment)

File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/018177f3_8fac6590
PS3, Line 515:  LOG_CNLINK(cnlink, DCN, LOGL_NOTICE, "using: cs7-%u %s <-> %s 
%s %s\n",
> Some context: this is the single "i am ready" notification for one SCCP link 
> to an MSC or SGSN. […]
multi-line logging is allways a PITA  from the point of anyone trying to 
process log files.  You cannot reasonably grep for context, if some part of a 
messge is printed int one line and some part of the same logical message is in 
another line.  You'd have to grep for one of the messages and then get back 
with some other tool to see the remainder of the message.

IMHO, we should avoid it at all costs.  Yes, there are some historical cases 
where we do have multi-line logging, but I consider those as "we were young and 
stupid" rather than a good example.

In order to work around the problem I added those LOGPC / DEBUGPC macros.  
Those worked fine back when all log targets were writing to stderr or to a 
file.  They break unfortunately with syslog, systemd or gsmtap logging, so they 
are discouraged, too.  Maybe we should actually OSMO_DEPRECATE them.



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 5
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: neels 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Thu, 15 Jun 2023 09:40:44 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels 
Comment-In-Reply-To: laforge 
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-12 Thread neels 
Attention is currently required from: laforge, pespin.

neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 5:

(2 comments)

File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/b4ea5e0a_a59e6401
PS3, Line 583:  hnbgw_cnlink_log_self(cnlink);
> what do I mean with extra log line? see here for instance, you end printing 
> twice, in line 581 and i […]
the reason are the distinct log levels.
In a live operation i don't want to see whether an SCCP instance existed 
before, but I might be interested in every new link to be printed once, hence 
on log level NOTICE.

And again, this happens exactly once per program startup, and once per MSC/SGSN.


https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/79c1d598_995dccb2
PS3, Line 635:  hnbgw_cnlink_log_self(cnlink);
> same here with line 612
same reason. DEBUG vs NOTICE



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 5
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: laforge 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Tue, 13 Jun 2023 01:05:00 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-08 Thread pespin 
Attention is currently required from: neels, laforge.

pespin has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 5: Code-Review+1

(2 comments)

File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/0aae164f_0d85e75b
PS3, Line 583:  hnbgw_cnlink_log_self(cnlink);
what do I mean with extra log line? see here for instance, you end printing 
twice, in line 581 and in line 583.


https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/5c86bbd7_f63c8ea1
PS3, Line 635:  hnbgw_cnlink_log_self(cnlink);
same here with line 612



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 5
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: neels 
Gerrit-Attention: laforge 
Gerrit-Comment-Date: Thu, 08 Jun 2023 08:19:30 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-07 Thread neels 
Attention is currently required from: laforge, pespin.

neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 4:

(2 comments)

File src/osmo-hnbgw/hnbgw.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/96cbece1_0acd35c4
PS3, Line 373:  result = talloc_asprintf(OTC_SELECT, "%s %s", result, 
ctx->identity_info);
> Ack
same answer as other patch: IMHO this is the best way.

I do not think changing this would have any noticeable impact on performance of 
osmo-hnbgw.

If it turns out to be performance critical, most definitely outputting the log 
line is much more heavy on load, and the only solution with any noticeable 
effect is anyway to scale down logging by cranking up category levels. You're 
of course aware that all of this code is skipped when no target will output the 
line.

Another approach you might suggest would be a 'name' cache added to struct 
hnb_context, and make sure to keep it updated as hnb_context members change. I 
would rather avoid that complexity: The current code is guaranteed to never 
print outdated items.

Have you guys thought this through? Am I missing something?


File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/1cf17292_fae68e7d
PS3, Line 515:  LOG_CNLINK(cnlink, DCN, LOGL_NOTICE, "using: cs7-%u %s <-> %s 
%s %s\n",
> Ack
Some context: this is the single "i am ready" notification for one SCCP link to 
an MSC or SGSN. This is called exactly once per 'msc N' and 'sgsn N' at program 
startup, at the point where the cn link to the msc or sgsn has resolved the cs7 
instance N that it will use, and local and remote point-codes for this CN peer. 
The function might be called again *only* when the user changes the SCCP 
configuration via telnet VTY so that SCCP links need to be restarted.
This would be a single 'LOG_CNLINK(...)' somewhere, but some scenarios reach 
this point in a different code path.

If you still think it is important to change this code:

wdym by 'extra log line'?
Do i understand this right: you are saying, it should be a macro to retain the 
caller's __FILE__ __LINE__ information?



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 4
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: laforge 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Thu, 08 Jun 2023 00:22:25 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge 
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-07 Thread laforge 
Attention is currently required from: neels, pespin.

laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 4:

(2 comments)

File src/osmo-hnbgw/hnbgw.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/f30bd2a4_d2e26ac6
PS3, Line 373:  result = talloc_asprintf(OTC_SELECT, "%s %s", result, 
ctx->identity_info);
> Again, I hope this is not called every time we log a line.
Ack


File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/0b2619f6_e248dec4
PS3, Line 515:  LOG_CNLINK(cnlink, DCN, LOGL_NOTICE, "using: cs7-%u %s <-> %s 
%s %s\n",
> This would rpboably better with a couple defines _FMT and _ARGS, so that it 
> ends up in same log line […]
Ack



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 4
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-CC: laforge 
Gerrit-Attention: neels 
Gerrit-Attention: pespin 
Gerrit-Comment-Date: Wed, 07 Jun 2023 09:50:20 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-06 Thread pespin 
Attention is currently required from: neels.

pespin has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 3: Code-Review+1

(2 comments)

File src/osmo-hnbgw/hnbgw.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/41119395_135ce299
PS3, Line 373:  result = talloc_asprintf(OTC_SELECT, "%s %s", result, 
ctx->identity_info);
Again, I hope this is not called every time we log a line.


File src/osmo-hnbgw/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/731cbadc_f57aa70b 
PS3, Line 515:  LOG_CNLINK(cnlink, DCN, LOGL_NOTICE, "using: cs7-%u %s <-> %s 
%s %s\n",
This would rpboably better with a couple defines _FMT and _ARGS, so that it 
ends up in same log line and not in an extra one.



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 3
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin 
Gerrit-Attention: neels 
Gerrit-Comment-Date: Tue, 06 Jun 2023 09:45:55 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-05 Thread neels 
Attention is currently required from: neels.

Hello Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171

to look at the new patch set (#3).

Change subject: tweak lots of logging
..

tweak lots of logging

Make lots of small tweaks in logging around RUA to SCCP context maps, CN
link selection and HNBAP:

- remove duplicate log context (e.g. LOGHNB() already shows the HNB Id)
- bring more sense into logging categories and levels / reduce noise
- add and clarify the details being logged at what points in time

Related: SYS#6412
Change-Id: I41275d8c3e272177976a9302795884666c35cd06
---
M include/osmocom/hnbgw/context_map.h
M src/osmo-hnbgw/context_map.c
M src/osmo-hnbgw/context_map_sccp.c
M src/osmo-hnbgw/hnbgw.c
M src/osmo-hnbgw/hnbgw_cn.c
M src/osmo-hnbgw/hnbgw_hnbap.c
M src/osmo-hnbgw/hnbgw_l3.c
M src/osmo-hnbgw/hnbgw_rua.c
8 files changed, 96 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/71/33171/3
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 3
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Attention: neels 
Gerrit-MessageType: newpatchset

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-05 Thread Jenkins Builder 
Attention is currently required from: neels.

Jenkins Builder has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 2:

(2 comments)

File src/osmo-hnbgw/context_map.c:

Robot Comment from checkpatch (run ID jenkins-gerrit-lint-7847):
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/d7f921bd_e8d09351
PS2, Line 133:   cnlink_selected->use.remote_addr_name ? " ": "",
spaces required around that ':' (ctx:VxW)


File src/osmo-hnbgw/hnbgw.c:

Robot Comment from checkpatch (run ID jenkins-gerrit-lint-7847):
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/6c2b1055_3b5ea3d9
PS2, Line 369:  else {
else should follow close brace '}'



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 2
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Attention: neels 
Gerrit-Comment-Date: Mon, 05 Jun 2023 21:01:38 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-05 Thread Jenkins Builder 
Jenkins Builder has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )

Change subject: tweak lots of logging
..


Patch Set 1:

(2 comments)

File src/osmo-hnbgw/context_map.c:

Robot Comment from checkpatch (run ID jenkins-gerrit-lint-7819):
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/f73b993c_fb301021
PS1, Line 133:   cnlink_selected->use.remote_addr_name ? " ": "",
spaces required around that ':' (ctx:VxW)


File src/osmo-hnbgw/hnbgw.c:

Robot Comment from checkpatch (run ID jenkins-gerrit-lint-7819):
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171/comment/9954bb46_9f2b2f9c
PS1, Line 369:  else {
else should follow close brace '}'



--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I41275d8c3e272177976a9302795884666c35cd06
Gerrit-Change-Number: 33171
Gerrit-PatchSet: 1
Gerrit-Owner: neels 
Gerrit-CC: Jenkins Builder
Gerrit-Comment-Date: Mon, 05 Jun 2023 13:28:38 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[M] Change in osmo-hnbgw[master]: tweak lots of logging

2023-06-05 Thread neels 
neels has uploaded this change for review. ( 
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33171 )


Change subject: tweak lots of logging
..

tweak lots of logging

Make lots of small tweaks in logging around RUA to SCCP context maps, CN
link selection and HNBAP:

- remove duplicate log context (e.g. LOGHNB() already shows the HNB Id)
- bring more sense into logging categories and levels / reduce noise
- add and clarify the details being logged at what points in time

Related: SYS#6412
Change-Id: I41275d8c3e272177976a9302795884666c35cd06
---
M include/osmocom/hnbgw/context_map.h
M src/osmo-hnbgw/context_map.c
M src/osmo-hnbgw/context_map_sccp.c
M src/osmo-hnbgw/hnbgw.c
M src/osmo-hnbgw/hnbgw_cn.c
M src/osmo-hnbgw/hnbgw_hnbap.c
M src/osmo-hnbgw/hnbgw_l3.c
M src/osmo-hnbgw/hnbgw_rua.c
8 files changed, 97 insertions(+), 41 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/71/33171/1

diff --git a/include/osmocom/hnbgw/context_map.h 
b/include/osmocom/hnbgw/context_map.h
index 7017491..f7335a7 100644
--- a/include/osmocom/hnbgw/context_map.h
+++ b/include/osmocom/hnbgw/context_map.h
@@ -7,8 +7,9 @@

 #define LOG_MAP(HNB_CTX_MAP, SUBSYS, LEVEL, FMT, ARGS...) \
LOGHNB((HNB_CTX_MAP) ? (HNB_CTX_MAP)->hnb_ctx : NULL, \
-  SUBSYS, LEVEL, "RUA-%u %s MI=%s%s%s: " FMT, \
+  SUBSYS, LEVEL, "RUA-%u SCCP-%u %s MI=%s%s%s: " FMT, \
   (HNB_CTX_MAP) ? (HNB_CTX_MAP)->rua_ctx_id : 0, \
+  (HNB_CTX_MAP) ? (HNB_CTX_MAP)->scu_conn_id : 0, \
   (HNB_CTX_MAP) ? \
((HNB_CTX_MAP)->cnlink ? (HNB_CTX_MAP)->cnlink->name \
  : ((HNB_CTX_MAP)->is_ps ? "PS" : "CS")) \
diff --git a/src/osmo-hnbgw/context_map.c b/src/osmo-hnbgw/context_map.c
index 13aae0e..045053c 100644
--- a/src/osmo-hnbgw/context_map.c
+++ b/src/osmo-hnbgw/context_map.c
@@ -26,6 +26,8 @@

 #include 

+#include 
+
 #include 

 #include 
@@ -92,7 +94,7 @@

llist_add_tail(&map->hnb_list, &hnb->map_list);

-   LOG_MAP(map, DRUA, LOGL_INFO, "New RUA CTX\n");
+   LOG_MAP(map, DRUA, LOGL_DEBUG, "New RUA CTX\n");
return map;
 }

@@ -117,12 +119,23 @@

map->cnlink = cnlink_selected;
map->scu_conn_id = new_scu_conn_id;
+   map_sccp_fsm_alloc(map);
+
llist_add_tail(&map->hnbgw_cnlink_entry, &cnlink_selected->map_list);

hash_add(hsu->hnbgw_context_map_by_conn_id, 
&map->hnbgw_sccp_user_entry, new_scu_conn_id);

-   LOG_MAP(map, DMAIN, LOGL_INFO, "Creating new Mapping RUA CTX %u <-> SCU 
Conn ID %u to %s on %s\n",
-   map->rua_ctx_id, new_scu_conn_id, cnlink_selected->name, 
hsu->name);
+   LOGP(DRUA, LOGL_NOTICE, "New conn: %s '%s' RUA-%u <-> SCCP-%u %s%s%s %s 
l=%s<->r=%s\n",
+osmo_sock_get_name2_c(OTC_SELECT, 
osmo_stream_srv_get_ofd(map->hnb_ctx->conn)->fd),
+hnb_context_name(map->hnb_ctx), map->rua_ctx_id,
+new_scu_conn_id,
+cnlink_selected->name,
+cnlink_selected->use.remote_addr_name ? " ": "",
+cnlink_selected->use.remote_addr_name ? : "",
+hsu->name,
+osmo_ss7_pointcode_print(hsu->ss7, hsu->local_addr.pc),
+osmo_ss7_pointcode_print2(hsu->ss7, 
cnlink_selected->remote_addr.pc)
+   );

return 0;
 }
diff --git a/src/osmo-hnbgw/context_map_sccp.c 
b/src/osmo-hnbgw/context_map_sccp.c
index 3bdec8c..1f2cd09 100644
--- a/src/osmo-hnbgw/context_map_sccp.c
+++ b/src/osmo-hnbgw/context_map_sccp.c
@@ -81,7 +81,8 @@
struct osmo_fsm_inst *fi = osmo_fsm_inst_alloc(&map_sccp_fsm, map, map, 
LOGL_DEBUG, NULL);
OSMO_ASSERT(fi);
osmo_fsm_inst_update_id_f_sanitize(fi, '-', "%s-%s-SCCP-%u", 
hnb_context_name(map->hnb_ctx),
-  map->is_ps ? "PS" : "CS", 
map->scu_conn_id);
+  map->cnlink ? map->cnlink->name : 
(map->is_ps ? "PS" : "CS"),
+  map->scu_conn_id);

OSMO_ASSERT(map->sccp_fi == NULL);
map->sccp_fi = fi;
diff --git a/src/osmo-hnbgw/hnbgw.c b/src/osmo-hnbgw/hnbgw.c
index f435043..dfeda53 100644
--- a/src/osmo-hnbgw/hnbgw.c
+++ b/src/osmo-hnbgw/hnbgw.c
@@ -351,13 +351,30 @@

 const char *hnb_context_name(struct hnb_context *ctx)
 {
+   char *result;
if (!ctx)
return "NULL";

+   if (ctx->conn) {
+   char hostbuf_r[INET6_ADDRSTRLEN];
+   char portbuf_r[6];
+   int fd = osmo_stream_srv_get_ofd(ctx->conn)->fd;
+
+   /* get remote addr */
+   if (osmo_sock_get_ip_and_port(fd, hostbuf_r, sizeof(hostbuf_r), 
portbuf_r, sizeof(portbuf_r), false) == 0)
+   result = talloc_asprintf(OTC_SELECT, "%s:%s", 
hostbuf_r, portbuf_r);
+   else
+   result = "?";
+   }
+   else {
+