Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11820 )
Change subject: LCLS: update parameter representation ...................................................................... Patch Set 2: Code-Review-1 (5 comments) https://gerrit.osmocom.org/#/c/11820/2/src/osmo-bsc/bsc_subscr_conn_fsm.c File src/osmo-bsc/bsc_subscr_conn_fsm.c: https://gerrit.osmocom.org/#/c/11820/2/src/osmo-bsc/bsc_subscr_conn_fsm.c@859 PS2, Line 859: /* initialize to some magic values that indicate "IE not [yet] received" */ (could drop the comment up to "indicate") https://gerrit.osmocom.org/#/c/11820/2/src/osmo-bsc/osmo_bsc_lcls.c File src/osmo-bsc/osmo_bsc_lcls.c: https://gerrit.osmocom.org/#/c/11820/2/src/osmo-bsc/osmo_bsc_lcls.c@164 PS2, Line 164: old_cfg_csc.config = conn->lcls.config; please use a construct like this: old_cfg_csc = (struct old_cfg_csc){ .config = conn->lcls.config, ... }; because that ensures that all struct members are initialized, and that there are no leftovers from a previous call. (because any member that isn't mentioned gets zero-initialized implicitly) https://gerrit.osmocom.org/#/c/11820/2/src/osmo-bsc/osmo_bsc_lcls.c@167 PS2, Line 167: if (new_cfg_csc->config != 0xff) { a magic nr left over https://gerrit.osmocom.org/#/c/11820/2/src/osmo-bsc/osmo_bsc_lcls.c@175 PS2, Line 175: if (new_cfg_csc->control != 0xff) { another magic nr https://gerrit.osmocom.org/#/c/11820/2/src/osmo-bsc/osmo_bsc_lcls.c@192 PS2, Line 192: old_cfg_csc = update_lcls_cfg_csc(conn, new_cfg_csc); unrelated to this particular patch, but how can this make sense if the returned cfg is never used anywhere?? Also I think the naming is weird, since the returned cfg should be the updated one, not the old one? If I'm right about this, could you plz create a new issue and possibly a patch... -- To view, visit https://gerrit.osmocom.org/11820 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e962d4fbb24bf1fb2398dc13e142a4a3304d858 Gerrit-Change-Number: 11820 Gerrit-PatchSet: 2 Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Wed, 21 Nov 2018 17:45:34 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
