Attention is currently required from: osmith, pespin, dexter.

fixeria has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-bsc/+/34192 )

Change subject: pcuif_proto: check confirm flag in struct gsm_pcu_if_pch
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

File src/osmo-bsc/pcu_sock.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/34192/comment/24b27257_6553e915
PS1, Line 552:          if (pch->confirmed_imm_ass)
> I disagree with this. […]
One can already see the consequences of making breaking changes "on the go" 
without bumping the protocol version: some testcases in ttcn3-bts-test started 
to fail because the testsuite is recent enough and including the `confirm` 
field, while current osmo-bts is not recent enough and rejecting DATA.req as 
malformed.

https://jenkins.osmocom.org/jenkins/view/TTCN3/job/ttcn3-bts-test/2135/ (+8 
failures)

Once this and the respective osmo-{pcu,bts} changes are merged, recent osmo-pcu 
speaking PCUIFv11 will become incompatible with older osmo-{bsc,bts} speaking 
the same [but different] PCUIFv11. And vice-versa.

As I said in 
https://gerrit.osmocom.org/c/osmo-bts/+/34059/comments/5a247c05_3683d840, I 
consider this a bad practice because this makes life harder for those using 
nightly and even harder for those running git-bisect. Protocol changes like 
this (and preceding ones) should be carefully planned, discussed, and merged 
all in one shot followed by a version bump. No breaking changes should be made 
after the version bump.

Don't take this as blaming or shaming, all I want to say is that we should 
avoid such situations in the future when changing these ad-hoc protocols. The 
patch itself looks good to me.



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

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I3d2842626b7e8325860ea3160c7d900d39e953a0
Gerrit-Change-Number: 34192
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Reviewer: osmith <[email protected]>
Gerrit-Reviewer: pespin <[email protected]>
Gerrit-Attention: osmith <[email protected]>
Gerrit-Attention: pespin <[email protected]>
Gerrit-Attention: dexter <[email protected]>
Gerrit-Comment-Date: Wed, 30 Aug 2023 22:42:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: osmith <[email protected]>
Comment-In-Reply-To: pespin <[email protected]>
Comment-In-Reply-To: dexter <[email protected]>
Gerrit-MessageType: comment

Reply via email to