Change in osmo-bsc[master]: LCLS: update parameter representation

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.org
Wed Nov 21 17:45:34 UTC 2018


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 <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 21 Nov 2018 17:45:34 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181121/f7db9266/attachment.htm>


More information about the gerrit-log mailing list