This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgPatch Set 3: > I would suggest not to jump into verdict before having proper > analysis of code. You're right that I had in fact not understood the code completely. Now that I'm reading it in more detail, I only find more problems. I will limit the comments here on the ones related to this patch: My point remains that in gprs_rlc_mcs_cps() the punct/punct2 are used in calculations directly without checking against INVALID items. By initializing both punct values to "INVALID" in create_dl_acked_block(), it is suggested that INVALID values may be passed on to gprs_rlc_mcs_cps() -- if that weren't the case, it would not be necessary to initialize to INVALID. So, gprs_rlc_mcs_cps() must check that it isn't using INVALID enum values in its bitfield calculations. Would it make more sense to set punct2 to INVALID explicitly in specific cases? Even though incidentally, the cases where one (or both??) args are INVALID might match the internal expectations of the switch() statement, this is bad coding style. Each function's API should make sense on its own. Thanks! -- To view, visit https://gerrit.osmocom.org/1411 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ice54edc7e4a936eb2f2dd8a243673a30dceef542 Gerrit-PatchSet: 3 Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Owner: arvind.sirsikar <arvind.sirsikar at radisys.com> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-Reviewer: arvind.sirsikar <arvind.sirsikar at radisys.com> Gerrit-HasComments: No