Change in osmo-bsc[master]: bsc_api.c: actually log with context

[Harald Welte]

Harald Welte has posted comments on this change. ( 
https://gerrit.osmocom.org/7996 )

Change subject: bsc_api.c: actually log with context
..


Patch Set 2: Code-Review+2


--
To view, visit https://gerrit.osmocom.org/7996
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If469defcc6fe8950dac5df61db3f39d297893318
Gerrit-Change-Number: 7996
Gerrit-PatchSet: 2
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Comment-Date: Wed, 23 May 2018 07:27:36 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes

Change in osmo-bsc[master]: bsc_api.c: actually log with context

[Harald Welte]

Harald Welte has submitted this change and it was merged. ( 
https://gerrit.osmocom.org/7996 )

Change subject: bsc_api.c: actually log with context
..

bsc_api.c: actually log with context

bsc_api.c notoriously lacks log context. Provide gsm_lchan_name() and/or
bsc_subscr_name() in roughly a million instances, using new LOGPLCHAN macro.

Add LOGPLCHAN() to gsm_data.h, to encourage use of it in other .c files.

Change-Id: If469defcc6fe8950dac5df61db3f39d297893318
---
M include/osmocom/bsc/gsm_data.h
M src/libbsc/bsc_api.c
2 files changed, 58 insertions(+), 43 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index 6f854d8..da5e87f 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -366,6 +366,14 @@
uint8_t key[MAX_A5_KEY_LEN];
 };

+#define LOGPLCHAN(lchan, ss, level, fmt, args...) \
+   LOGP(ss, level, "%s (ss=%d,%s) (%s) " fmt, \
+lchan ? gsm_ts_and_pchan_name(lchan->ts) : "-", \
+lchan ? lchan->nr : 0, \
+lchan ? gsm_lchant_name(lchan->type) : "-", \
+bsc_subscr_name(lchan && lchan->conn ? lchan->conn->bsub : NULL), \
+## args)
+
 struct gsm_lchan {
/* The TS that we're part of */
struct gsm_bts_trx_ts *ts;
diff --git a/src/libbsc/bsc_api.c b/src/libbsc/bsc_api.c
index 13fe099..71cc0f0 100644
--- a/src/libbsc/bsc_api.c
+++ b/src/libbsc/bsc_api.c
@@ -117,17 +117,17 @@
new_lchan = lchan_alloc(conn_get_bts(conn), chan_type, 0);

if (!new_lchan) {
-   LOGP(DMSC, LOGL_NOTICE, "No free channel.\n");
+   LOGP(DMSC, LOGL_NOTICE, "%s No free channel for %s\n",
+bsc_subscr_name(conn->bsub), gsm_lchant_name(chan_type));
return -1;
}

/* check if we are on TCH/F and requested TCH/H, but got TCH/F */
if (conn->lchan->type == new_lchan->type
&& chan_type != new_lchan->type) {
-   LOGP(DHO, LOGL_NOTICE, "%s -> %s Will not re-assign to 
identical channel type,"
-" %s was requested\n",
-gsm_lchan_name(conn->lchan), gsm_lchan_name(new_lchan),
-gsm_lchant_name(chan_type));
+   LOGPLCHAN(conn->lchan, DHO, LOGL_NOTICE,
+ "-> %s Will not re-assign to identical channel type, 
%s was requested\n",
+ gsm_lchan_name(new_lchan), 
gsm_lchant_name(chan_type));
lchan_free(new_lchan);
return -1;
}
@@ -148,7 +148,7 @@
handle_mr_config(conn, new_lchan, full_rate);

if (rsl_chan_activate_lchan(new_lchan, 0x1, 0) < 0) {
-   LOGP(DHO, LOGL_ERROR, "could not activate channel\n");
+   LOGPLCHAN(new_lchan, DHO, LOGL_ERROR, "could not activate 
channel\n");
lchan_free(new_lchan);
return -1;
}
@@ -217,7 +217,8 @@

if (!conn->lchan) {
LOGP(DMSC, LOGL_ERROR,
-"Called submit dtap without an lchan.\n");
+"%s Called submit dtap without an lchan.\n",
+bsc_subscr_name(conn->bsub));
msgb_free(msg);
return -1;
}
@@ -320,11 +321,9 @@
if (chan_mode == GSM48_CMODE_SPEECH_AMR)
handle_mr_config(conn, conn->lchan, full_rate);

-   LOGP(DMSC, LOGL_NOTICE,
-"Sending %s ChanModify for speech: %s on channel %s\n",
-gsm_lchan_name(conn->lchan),
-get_value_string(gsm48_chan_mode_names, chan_mode),
-get_value_string(gsm_chan_t_names, conn->lchan->type));
+   LOGPLCHAN(conn->lchan, DMSC, LOGL_NOTICE,
+ "Sending ChanModify for speech: %s\n",
+ get_value_string(gsm48_chan_mode_names, chan_mode));
gsm48_lchan_modify(conn->lchan, chan_mode);
}

@@ -352,8 +351,8 @@
struct lchan_signal_data sig;
struct gsm48_hdr *gh = msgb_l3(msg);

-   DEBUGP(DRR, "ASSIGNMENT COMPLETE cause = %s\n",
-   rr_cause_name(gh->data[0]));
+   LOGPLCHAN(msg->lchan, DRR, LOGL_DEBUG, "ASSIGNMENT COMPLETE 
cause = %s\n",
+ rr_cause_name(gh->data[0]));

sig.lchan = msg->lchan;
sig.mr = NULL;
@@ -367,14 +366,15 @@
}

if (conn->secondary_lchan != msg->lchan) {
-   LOGP(DMSC, LOGL_ERROR, "Assignment Compl should occur on second 
lchan.\n");
+   LOGPLCHAN(msg->lchan, DMSC, LOGL_ERROR,
+ "Assignment Compl should occur on second lchan.\n");
return;
}

gh = msgb_l3(msg);
   

Change in osmo-bsc[master]: bsc_api.c: actually log with context

[Neels Hofmeyr]

Hello Harald Welte, Jenkins Builder,

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

https://gerrit.osmocom.org/7996

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

Change subject: bsc_api.c: actually log with context
..

bsc_api.c: actually log with context

bsc_api.c notoriously lacks log context. Provide gsm_lchan_name() and/or
bsc_subscr_name() in roughly a million instances, using new LOGPLCHAN macro.

Add LOGPLCHAN() to gsm_data.h, to encourage use of it in other .c files.

Change-Id: If469defcc6fe8950dac5df61db3f39d297893318
---
M include/osmocom/bsc/gsm_data.h
M src/libbsc/bsc_api.c
2 files changed, 58 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/96/7996/2
--
To view, visit https://gerrit.osmocom.org/7996
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If469defcc6fe8950dac5df61db3f39d297893318
Gerrit-Change-Number: 7996
Gerrit-PatchSet: 2
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 

Change in osmo-bsc[master]: bsc_api.c: actually log with context

[Neels Hofmeyr]

Neels Hofmeyr has posted comments on this change. ( 
https://gerrit.osmocom.org/7996 )

Change subject: bsc_api.c: actually log with context
..


Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/#/c/7996/1/src/libbsc/bsc_api.c
File src/libbsc/bsc_api.c:

https://gerrit.osmocom.org/#/c/7996/1/src/libbsc/bsc_api.c@47
PS1, Line 47: #define LOGPLCHAN(lchan, ss, level, fmt, args...) \
> I think this should go in a header file so it can be used from more code
agreed



--
To view, visit https://gerrit.osmocom.org/7996
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If469defcc6fe8950dac5df61db3f39d297893318
Gerrit-Change-Number: 7996
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Comment-Date: Thu, 17 May 2018 16:36:36 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

Change in osmo-bsc[master]: bsc_api.c: actually log with context

[Harald Welte]

Harald Welte has posted comments on this change. ( 
https://gerrit.osmocom.org/7996 )

Change subject: bsc_api.c: actually log with context
..


Patch Set 1: Code-Review+2

(1 comment)

https://gerrit.osmocom.org/#/c/7996/1/src/libbsc/bsc_api.c
File src/libbsc/bsc_api.c:

https://gerrit.osmocom.org/#/c/7996/1/src/libbsc/bsc_api.c@47
PS1, Line 47: #define LOGPLCHAN(lchan, ss, level, fmt, args...) \
I think this should go in a header file so it can be used from more code



--
To view, visit https://gerrit.osmocom.org/7996
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If469defcc6fe8950dac5df61db3f39d297893318
Gerrit-Change-Number: 7996
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Comment-Date: Tue, 15 May 2018 20:59:26 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes