March 2023 - gerrit-log - lists.osmocom.org

[S] Change in libosmo-gprs[master]: rlcmac: rlcmac_dec.c: Add missing headers
by fixeria 08 Mar '23

08 Mar '23

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/31752 ) Change subject: rlcmac: rlcmac_dec.c: Add missing headers ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/31752 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-gprs Gerrit-Branch: master Gerrit-Change-Id: Ie7fc1a8b101d8ee978a6a770ffe0190fb0de605a Gerrit-Change-Number: 31752 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:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in libosmo-gprs[master]: rlcmac: ul_tbf: Implement support for TBF Starting Time
by fixeria 08 Mar '23

08 Mar '23

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

1 0

[M] Change in libosmo-gprs[master]: rlcmac: Lower logging rx prim from lower layers
by fixeria 08 Mar '23

08 Mar '23

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/31733 ) Change subject: rlcmac: Lower logging rx prim from lower layers ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/31733 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-gprs Gerrit-Branch: master Gerrit-Change-Id: I0b13fa374003a7c48bc259a8c45227b86beca454 Gerrit-Change-Number: 31733 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:51:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in libosmo-gprs[master]: rlcmac: Ignore DATA.ind with len=0
by fixeria 08 Mar '23

08 Mar '23

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

1 0

[S] Change in libosmo-gprs[master]: rlcmac: Ignore DATA.ind with len=0
by fixeria 08 Mar '23

08 Mar '23

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+2 -- 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:49:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in libosmocore[master]: GSMTAP: allow configuring src IP for log messages
by fixeria 08 Mar '23

08 Mar '23

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

1 0

[S] Change in libosmocore[master]: osmo_wqueue_enqueue(): Avoid redundant if-check
by msuraev 08 Mar '23

08 Mar '23

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

1 0

[S] Change in osmo-gsm-manuals[master]: build/Makefile.common.inc: unset OSMO_REPOSITORY
by msuraev 08 Mar '23

08 Mar '23

Attention is currently required from: osmith, fixeria. msuraev has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/31726 ) Change subject: build/Makefile.common.inc: unset OSMO_REPOSITORY ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/31726 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-gsm-manuals Gerrit-Branch: master Gerrit-Change-Id: Ie21d58dd57a893a83f566eea464ab3eec82b67a4 Gerrit-Change-Number: 31726 Gerrit-PatchSet: 2 Gerrit-Owner: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Wed, 08 Mar 2023 18:35:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in libosmocore[master]: GSMTAP: allow configuring src IP for log messages
by msuraev 08 Mar '23

08 Mar '23

Attention is currently required from: osmith, laforge, pespin, fixeria. msuraev 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 10: (2 comments) This change is ready for review. File include/osmocom/core/logging.h: https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/5f4afe9a_4eca6008 PS8, Line 354: char *src; > src_addr Done File src/vty/logging_vty.c: https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/3d532d78_e72dc60d PS8, Line 815: tgt = log_target_find(LOG_TGT_TYPE_GSMTAP, hostname); > Now if you do: […] Done -- 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: 10 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: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Wed, 08 Mar 2023 18:23:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmocom-bb[master]: mobile: allow configuring local GSMTAP address
by fixeria 08 Mar '23

08 Mar '23

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

1 0