December 2022 - gerrit-log - lists.osmocom.org

Change in osmo-pcu[master]: Avoid moving DL-TBF from old_msg to new_ms during ms_merge

by osmith

Attention is currently required from: laforge, pespin, fixeria. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/30640 ) Change subject: Avoid moving DL-TBF from old_msg to new_ms during ms_merge ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/30640 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ie8cb49d5492cfc4cbf8340f3f376b0e6105e8c82 Gerrit-Change-Number: 30640 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(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-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 11:33:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-pcu[master]: tests/TbfTest: reproduce buggy corner case: MS with TBFs on 2 TRXs

by pespin

Attention is currently required from: osmith, laforge, fixeria. Hello osmith, Jenkins Builder, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/30639 to look at the new patch set (#3). Change subject: tests/TbfTest: reproduce buggy corner case: MS with TBFs on 2 TRXs ...................................................................... tests/TbfTest: reproduce buggy corner case: MS with TBFs on 2 TRXs Add a test which showcases a scenario where the PCU ends up with 1 GprsMs object holding a DL-TBF in a weird condition half in one TRX and half in other due to ms_merge(). This test (slightly adapted) used to cause a crash in osmo-pcu.git 586ddda9bc09d60f2d491158de843825cb7c876a (a few versions behind current master). Related: SYS#6231 Change-Id: Ic16b5e96debf91e72684833cd64253687857f3aa --- M tests/tbf/TbfTest.cpp M tests/tbf/TbfTest.err 2 files changed, 207 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/39/30639/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/30639 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ic16b5e96debf91e72684833cd64253687857f3aa Gerrit-Change-Number: 30639 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(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-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: newpatchset

2 years, 6 months

1
0
0 0

Change in osmo-pcu[master]: Avoid moving DL-TBF from old_msg to new_ms during ms_merge

by pespin

Attention is currently required from: osmith, laforge, fixeria. Hello osmith, Jenkins Builder, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/30640 to look at the new patch set (#3). Change subject: Avoid moving DL-TBF from old_msg to new_ms during ms_merge ...................................................................... Avoid moving DL-TBF from old_msg to new_ms during ms_merge The DL-TBF assigned to another MS object may have a totally different set of reserved resources (TS set, TRX, etc.), so one cannot simply move those to the new MS. To start with, if the 2 MS are on different TRX it is clear that one of them will not be really in operation. That's most probably the DL-TBF being in ASSIGN state on CCCH waiting for PCUIF_CNF and later X2002 to trigger to start sending DL blocks, but without confirmation whether the MS is really there. Since the other new MS object probably has a UL-TBF, that's the one probably operative, and hence a new DL-TBF can be created at that same time and assigned through UL-TBF's PACCH. Unit test test_ms_merge_dl_tbf_different_trx showcases the above scenario. Related: SYS#6231 Related: OS#5700 Related: 677bebbe5c49d4607322e96053fe14ddd78d9549 Change-Id: Ie8cb49d5492cfc4cbf8340f3f376b0e6105e8c82 --- M src/gprs_ms.c M src/tbf_dl_fsm.c M tests/tbf/TbfTest.cpp M tests/tbf/TbfTest.err 4 files changed, 98 insertions(+), 61 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/40/30640/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/30640 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ie8cb49d5492cfc4cbf8340f3f376b0e6105e8c82 Gerrit-Change-Number: 30640 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(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-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: newpatchset

2 years, 6 months

1
0
0 0

Change in osmo-pcu[master]: tests/TbfTest: reproduce buggy corner case: MS with TBFs on 2 TRXs

by pespin

Attention is currently required from: osmith, laforge, fixeria. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/30639 ) Change subject: tests/TbfTest: reproduce buggy corner case: MS with TBFs on 2 TRXs ...................................................................... Patch Set 2: (1 comment) File tests/tbf/TbfTest.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/30639/comment/6cdfdf49_713b8b2e PS2, Line 2481: /* Here we assert DL-TBF is currently moved to the new MS, which is wrong! */ > maybe put a FIXME here? It doesn't really matter since it gets fixed in follow-up commit :) -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/30639 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ic16b5e96debf91e72684833cd64253687857f3aa Gerrit-Change-Number: 30639 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(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-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 11:31:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith <osmith(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-pcu[master]: tests/TbfTest: reproduce buggy corner case: MS with TBFs on 2 TRXs

by osmith

Attention is currently required from: laforge, pespin, fixeria. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/30639 ) Change subject: tests/TbfTest: reproduce buggy corner case: MS with TBFs on 2 TRXs ...................................................................... Patch Set 2: Code-Review+1 (3 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-pcu/+/30639/comment/343e3abc_25ed5d0e PS2, Line 10: hald half Patchset: PS2: very readable test case, nice File tests/tbf/TbfTest.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/30639/comment/ef1d97c0_b125593a PS2, Line 2481: /* Here we assert DL-TBF is currently moved to the new MS, which is wrong! */ maybe put a FIXME here? -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/30639 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ic16b5e96debf91e72684833cd64253687857f3aa Gerrit-Change-Number: 30639 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(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-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 11:29:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-bts[master]: oc2gbts_mgr_calib: Don't cast timespec_t to time_t

by osmith

Attention is currently required from: pespin, daniel. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/30638 ) Change subject: oc2gbts_mgr_calib: Don't cast timespec_t to time_t ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Without looking into it I guess the variable changed type based on gpsd version? yep, we'll need to check with #if GPSD_API_MAJOR_VERSION >= 9. I'll submit a new version of this patch as part of OS#5832. -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/30638 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I6dc8ce303e5cb0fb412857a7f2c925e8cfe9b1e0 Gerrit-Change-Number: 30638 Gerrit-PatchSet: 1 Gerrit-Owner: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-CC: msuraev <msuraev(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 11:16:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-bsc[master]: WIP: vty: Add check against sensible default value for Ny1

by pespin

Attention is currently required from: arehbein. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 ) Change subject: WIP: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 9: (2 comments) File tests/nanobts_omlattr/nanobts_omlattr_test.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/577ed8cd_b305f504 PS9, Line 133: { .T = -3105, .default_val = GSM_NY1_DEFAULT, .val = NY1_TESTVAL, .unit = OSMO_TDEF_CUSTOM, > My understanding from the comments above is that you want to set Ny1 to have it applied in the attr_ […] Ah I see now that you actually this this T3015 here because it's used when test calls the check_ny1 function. https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/bd8004c0_205e5851 PS9, Line 161: trx = talloc_zero(ctx, struct gsm_bts_trx); I think this test is missing a calling to osmo_tdefs_reset(net->T_defs); -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 9 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 10:58:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-bsc[master]: WIP: vty: Add check against sensible default value for Ny1

by pespin

Attention is currently required from: arehbein. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 ) Change subject: WIP: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 9: (13 comments) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/594c7528_17c128f5 PS9, Line 535: if (!gsm_bts_check_ny1(bts, LOGL_ERROR, "Value of Ny1 too low.")) { This string can probably go inside the gsm_bts_check_ny1 function. https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/7d9f4a2b_889360b2 PS9, Line 1725: /* Return 'true' iff ny1 satisfies network requirements, otherwise return a suggestion for ny1. "Ny1" (see initial cap). BTW I think this comment needs to be updated, I don't see it returning any "suggestion". https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/8dbb7c58_755159a9 PS9, Line 1727: bool gsm_bts_check_ny1(struct gsm_bts *bts, unsigned int log_level, char const *msg) const always first. Also, "msg" name is usually used for struct msgb, better call it "str". https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/06324143_a7020bb8 PS9, Line 1729: struct gsm_lchan *gl = gsm_bts_get_cbch(bts); we usually use "lchan" as variable name for a struct gsm_lchan. Why are you getting a CBCH lchan here btw? https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/7f753c77_4c7e325d PS9, Line 1738: unsigned long T3105 = osmo_tdef_get_entry(bts->network->T_defs, 3105)->val; You can use osmo_tdef_get() here. Passing -1 to "val_if_not_present" param akes care of proper exiting if the timer is not found (shouldn't happen). https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/0c071ad3_7c42de9e PS9, Line 1740: unsigned long ny1 = osmo_tdef_get_entry(bts->network->T_defs, -3105)->val; Same, osmo_tdef_get() https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/a5045235_1cecaf44 PS9, Line 1742: bool ny1_satisfies_requirements = T3105 * ny1 > T3124 + GSM_NY1_REQ_DELTA; We usually declare variables at the start of the function/block. https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/856f773a_855077d4 PS9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1; Why this +1? can you explain? https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/11c8a152_b2b01c89 PS9, Line 1748: return ny1_satisfies_requirements; You can simplify all this code with early return: if (T3105 * ny1 < T3124 + GSM_NY1_REQ_DELTA) { unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1; LOGP(DNM, log_level, "%s Is: %lu Lowest recommendation: %lu\n", msg, ny1, ny1_recommended); return false; } return true; File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/7767e628_93eaad1c PS9, Line 96: (void) gsm_bts_check_ny1(bts, LOGL_NOTICE, "Value of Ny1 should be higher."); In one function is "should be lower" and here "should be higher"? This probably makes no sense. File tests/nanobts_omlattr/nanobts_omlattr_test.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/642a3ec1_9fa75aec PS9, Line 127: static unsigned long const T3105_TESTVAL = 13; > Move const after static - use 'static const unsigned long' Please fix. const always at the start (well, after "static" in this case as the linter says). https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/16fd2c9f_9bb0546c PS9, Line 130: static unsigned long const NY1_TESTVAL = (GSM_T3124_MAX + GSM_NY1_REQ_DELTA)/T3105_TESTVAL + 1; > Move const after static - use 'static const unsigned long' Please fix. https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/591c2218_e53633da PS9, Line 133: { .T = -3105, .default_val = GSM_NY1_DEFAULT, .val = NY1_TESTVAL, .unit = OSMO_TDEF_CUSTOM, My understanding from the comments above is that you want to set Ny1 to have it applied in the attr_bts_expected[] below. That's fine. If T3105 is not needed in the test, then simply don't set it and drop the comment. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 9 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 10:51:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-msc[master]: SMPP: refactor ref counting code

by pespin

Attention is currently required from: msuraev. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/30683 ) Change subject: SMPP: refactor ref counting code ...................................................................... Patch Set 1: Code-Review-1 (1 comment) Patchset: PS1: I don't see the point of this patch. It's way clearer having a separate static function holding all the destroy code. My understanding is that fixeria also agrees with that? -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/30683 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I9cb530991f54aa78edaa885f1242f63c705b6fcb Gerrit-Change-Number: 30683 Gerrit-PatchSet: 1 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 10:34:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-msc[master]: SMPP: use proper type for boolean variables

by pespin

Attention is currently required from: msuraev. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/30684 ) Change subject: SMPP: use proper type for boolean variables ...................................................................... Patch Set 1: Code-Review-2 (1 comment) Patchset: PS1: I'm happy that you use bool, but don't merge those into a single line. -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/30684 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I033a2c863213a44f99827ef997f541a7a77f5d8a Gerrit-Change-Number: 30684 Gerrit-PatchSet: 1 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Comment-Date: Mon, 19 Dec 2022 10:33:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

2025

2024

2023

2022

gerrit-log December 2022