Change in osmo-pcu[master]: encoding: assert() presence of Downlink TBF

2020-08-06 Thread fixeria 
fixeria has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/19310 )

Change subject: encoding: assert() presence of Downlink TBF
..


Patch Set 1:

(1 comment)

Sorry for late reply, somehow I missed these comments. Given that this is not 
supposed to happen, logging is not really important in this case. I really like 
the idea of sending crash indications over GSMTAP though.

https://gerrit.osmocom.org/c/osmo-pcu/+/19310/1/src/encoding.cpp
File src/encoding.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/19310/1/src/encoding.cpp@497
PS1, Line 497:  OSMO_ASSERT(as_dl_tbf(tbf) != NULL);
> UNRELATED: maybe we should make sure that osmo_panic_default() sends a GSMTAP 
> packet, if gsmtap logg […]
Indeed, this is an interesting idea. Especially if this "crash indication" 
would additionally contain the backtrace. We should definitely create a ticket.



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

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
Gerrit-Change-Number: 19310
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria 
Gerrit-Reviewer: laforge 
Gerrit-CC: pespin 
Gerrit-Comment-Date: Thu, 06 Aug 2020 17:33:33 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin 
Comment-In-Reply-To: laforge 
Gerrit-MessageType: comment

Change in osmo-pcu[master]: encoding: assert() presence of Downlink TBF

2020-08-06 Thread laforge 
laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/19310 )

Change subject: encoding: assert() presence of Downlink TBF
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
Gerrit-Change-Number: 19310
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-CC: pespin 
Gerrit-Comment-Date: Thu, 06 Aug 2020 16:49:14 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in osmo-pcu[master]: encoding: assert() presence of Downlink TBF

2020-08-06 Thread laforge 
laforge has submitted this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/19310 )

Change subject: encoding: assert() presence of Downlink TBF
..

encoding: assert() presence of Downlink TBF

This is not something that should normally happen.  If it happens,
then it's definitely a bug, and we should not tolerate it.

Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
---
M src/encoding.cpp
1 file changed, 1 insertion(+), 4 deletions(-)

Approvals:
  Jenkins Builder: Verified
  laforge: Looks good to me, approved



diff --git a/src/encoding.cpp b/src/encoding.cpp
index 2564575..9dfd7c9 100644
--- a/src/encoding.cpp
+++ b/src/encoding.cpp
@@ -494,10 +494,7 @@
/* 3GPP TS 44.018 §10.5.2.16 IA Rest Octets */
dest->cur_bit = wp;
if (downlink) {
-   if (!as_dl_tbf(tbf)) {
-   LOGP(DRLCMACDL, LOGL_ERROR, "Cannot encode DL IMMEDIATE 
ASSIGNMENT without TBF\n");
-   return -EINVAL;
-   }
+   OSMO_ASSERT(as_dl_tbf(tbf) != NULL);

rc = write_ia_rest_downlink(as_dl_tbf(tbf), dest, polling, 
gsm48_ta_is_valid(ta), fn, alpha, gamma,
ta_idx);

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

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
Gerrit-Change-Number: 19310
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-CC: pespin 
Gerrit-MessageType: merged

Change in osmo-pcu[master]: encoding: assert() presence of Downlink TBF

2020-07-18 Thread laforge 
laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/19310 )

Change subject: encoding: assert() presence of Downlink TBF
..


Patch Set 1: Code-Review+1

(1 comment)

https://gerrit.osmocom.org/c/osmo-pcu/+/19310/1/src/encoding.cpp
File src/encoding.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/19310/1/src/encoding.cpp@497
PS1, Line 497:  OSMO_ASSERT(as_dl_tbf(tbf) != NULL);
> what about keeping the log msg? it may be useful for instance if gsmtap log 
> is used.
UNRELATED: maybe we should make sure that osmo_panic_default() sends a GSMTAP 
packet, if gsmtap logging is enabled?  This way the infrastructure takes care 
of it, not having to rely on developers sending an explicit log message before 
OSMO_ASSERT()?



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

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
Gerrit-Change-Number: 19310
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-CC: pespin 
Gerrit-Comment-Date: Sat, 18 Jul 2020 21:42:16 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin 
Gerrit-MessageType: comment

Change in osmo-pcu[master]: encoding: assert() presence of Downlink TBF

2020-07-18 Thread pespin 
pespin has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/19310 )

Change subject: encoding: assert() presence of Downlink TBF
..


Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/c/osmo-pcu/+/19310/1/src/encoding.cpp
File src/encoding.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/19310/1/src/encoding.cpp@497
PS1, Line 497:  OSMO_ASSERT(as_dl_tbf(tbf) != NULL);
what about keeping the log msg? it may be useful for instance if gsmtap log is 
used.



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

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
Gerrit-Change-Number: 19310
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria 
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin 
Gerrit-Comment-Date: Sat, 18 Jul 2020 16:29:32 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in osmo-pcu[master]: encoding: assert() presence of Downlink TBF

2020-07-18 Thread fixeria 
fixeria has uploaded this change for review. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/19310 )


Change subject: encoding: assert() presence of Downlink TBF
..

encoding: assert() presence of Downlink TBF

This is not something that should normally happen.  If it happens,
then it's definitely a bug, and we should not tolerate it.

Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
---
M src/encoding.cpp
1 file changed, 1 insertion(+), 4 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/10/19310/1

diff --git a/src/encoding.cpp b/src/encoding.cpp
index 2564575..9dfd7c9 100644
--- a/src/encoding.cpp
+++ b/src/encoding.cpp
@@ -494,10 +494,7 @@
/* 3GPP TS 44.018 §10.5.2.16 IA Rest Octets */
dest->cur_bit = wp;
if (downlink) {
-   if (!as_dl_tbf(tbf)) {
-   LOGP(DRLCMACDL, LOGL_ERROR, "Cannot encode DL IMMEDIATE 
ASSIGNMENT without TBF\n");
-   return -EINVAL;
-   }
+   OSMO_ASSERT(as_dl_tbf(tbf) != NULL);

rc = write_ia_rest_downlink(as_dl_tbf(tbf), dest, polling, 
gsm48_ta_is_valid(ta), fn, alpha, gamma,
ta_idx);

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

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I6e46ba42650f0db2399649b536a1d2b3f0fcbf04
Gerrit-Change-Number: 19310
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria 
Gerrit-MessageType: newchange