Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/31751 )
Change subject: rlcmac: ul_tbf: Implement support for TBF Starting Time
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
File include/osmocom/gprs/rlcmac/rlcmac_dec.h:
https://gerrit.osmocom.org/c/libosmo-gprs/+/31751/comment/30e692b6_0a4fd5e3
PS1, Line 8: #include <osmocom/gsm/gsm0502.h>
: #include <osmocom/gsm/gsm_utils.h>
Why are you including these headers? I guess because some stuff is needed for the `sched.h`? If so, then `sched.h` needs to be fixed to include the necessary headers.
File src/rlcmac/rlcmac.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/31751/comment/7f30accc_32d83518
PS1, Line 194: uint32_t fn
Unused?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/31751
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: Id81f16743f2c464e01caf27ba2eb8c0fc715fe8a
Gerrit-Change-Number: 31751
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 08 Mar 2023 19:02:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/31732 )
Change subject: rlcmac: Ignore DATA.ind with len=0
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File src/rlcmac/rlcmac_prim.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/31732/comment/c0cf2c7a_ca956a56
PS1, Line 473: if (rlcmac_prim->l1ctl.pdch_data_ind.data_len == 0) {
Maybe use `OSMO_UNLIKELY` here?
I am planning to change the modem app to not send DATA.ind with len=0 anyway.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/31732
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: I342c7999261832a5d981e5831e428d2cbd82729e
Gerrit-Change-Number: 31732
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 08 Mar 2023 18:50:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: osmith, laforge, pespin, msuraev.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31513 )
Change subject: GSMTAP: allow configuring src IP for log messages
......................................................................
Patch Set 11: Code-Review-1
(6 comments)
Patchset:
PS11:
CR-1 due to the memleak.
File include/osmocom/core/logging.h:
https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/dce9223e_6518e0ad
PS11, Line 414: log_set_src_ip
This is the GSMTAP target specific API, so I believe it should be reflected in the symbol name, e.g. `log_set_gsmtap_src_ip`.
https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/0a235fe2_e7611287
PS11, Line 444: log_target_create_gsmtap2
Now that you're adding `log_set[_gamtap]_src_ip()`, do we really need this new API? Wouldn't it be enough to call the old `log_target_create_gsmtap()` and then immediately `log_set[_gamtap]_src_ip()`?
Ah, now I see that `log_set[_gamtap]_src_ip()` is using `gsmtap_source_init2()` internally.
File src/core/logging_gsmtap.c:
https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/62b526a5_1a76b538
PS11, Line 127: ip
I think it can be a hostname, not necessarily an IP address?
For instance, you can use 'localhost' and it should work.
Not 100% sure though.
https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/401bfc49_fbee37ef
PS11, Line 135: gsmtap_source_init2
Looking at the code I can see `gsmtap_source_init2()` can handle `src == NULL`, so why not calling it unconditionally?
https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/02d9c132_a77f97ea
PS11, Line 145: target->tgt_gsmtap.src_addr
What if `src_addr` was set before? You will be leaking memory here.
Better use `osmo_talloc_replace_string()`.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31513
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I9000269ce5b3dce1e757271b7c42e77b68d38f25
Gerrit-Change-Number: 31513
Gerrit-PatchSet: 11
Gerrit-Owner: msuraev <msuraev(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: msuraev <msuraev(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 08 Mar 2023 18:44:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
msuraev has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31591 )
Change subject: osmo_wqueue_enqueue(): Avoid redundant if-check
......................................................................
Patch Set 2: Code-Review-1
(1 comment)
Patchset:
PS2:
> The result is basically one function less on the call stack and one
if-check less
Only if we turn off all compiler optimisations. Which pretty-much never happens in practice so the actual result is the code which is harder to read.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31591
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I3ecc1698e836ca40e178a5b84c2946ae0e6bbfc9
Gerrit-Change-Number: 31591
Gerrit-PatchSet: 2
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 08 Mar 2023 18:42:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: msuraev.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31661 )
Change subject: mobile: allow configuring local GSMTAP address
......................................................................
Patch Set 5:
(1 comment)
File src/host/layer23/src/common/vty.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/31661/comment/40f47863_8fb7e509
PS5, Line 199: Local IP address
Now you have incomplete documentation for `VTY_IPV46_CMD`, which requires two docstrings. Are you testing it before merging at all? Please submit a fixup.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31661
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Ia1555db653cf0bb20af74617f33aad31c971bfdb
Gerrit-Change-Number: 31661
Gerrit-PatchSet: 5
Gerrit-Owner: msuraev <msuraev(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: msuraev <msuraev(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 08 Mar 2023 18:23:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment