Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12377 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 5: Code-Review-2 see my other comment on the libosmocore side of this change. -- To view, visit https://gerrit.osmocom.org/12377 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc3a2896b133298cbf850d68e6898300884bbce Gerrit-Change-Number: 12377 Gerrit-PatchSet: 5 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Sat, 29 Dec 2018 20:12:31 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Max has posted comments on this change. ( https://gerrit.osmocom.org/12377 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 5: > lcls_bss_status_present is a field in a public structure, so it's part of the > public API right? Right but it's removed in https://gerrit.osmocom.org/c/libosmocore/+/12369 and not in this commit. > If it was already present in an older release, then we should avoid changing > it to prevent from API breakage. If it was included after latest release, > it's fine to change it since it was never released. If we would follow this logic we'll keep broken API forever which does not make much sense and that's not how we handled this so far. Instead we should properly bump the API/ABI version. To do that we update TODO-RELEASE which is done in corresponding commit. -- To view, visit https://gerrit.osmocom.org/12377 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc3a2896b133298cbf850d68e6898300884bbce Gerrit-Change-Number: 12377 Gerrit-PatchSet: 5 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Sat, 29 Dec 2018 18:59:38 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12377 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 5: > > provide information on when was this API added > > Which API are you talking about? And how the time of addition is > relevent to whether we should use it or not? lcls_bss_status_present is a field in a public structure, so it's part of the public API right? Time of addition matters. If it was already present in an older release, then we should avoid changing it to prevent from API breakage. If it was included after latest release, it's fine to change it since it was never released. -- To view, visit https://gerrit.osmocom.org/12377 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc3a2896b133298cbf850d68e6898300884bbce Gerrit-Change-Number: 12377 Gerrit-PatchSet: 5 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Sat, 29 Dec 2018 17:59:13 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Max has posted comments on this change. ( https://gerrit.osmocom.org/12377 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 5: > provide information on when was this API added Which API are you talking about? And how the time of addition is relevent to whether we should use it or not? -- To view, visit https://gerrit.osmocom.org/12377 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc3a2896b133298cbf850d68e6898300884bbce Gerrit-Change-Number: 12377 Gerrit-PatchSet: 5 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Fri, 28 Dec 2018 14:38:59 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12377 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 5: Code-Review-1 This is still an API change because the boolean struct field is not used anymore, which means the way you are expected to use the API changed. Please again, provide information on when was this API added in order to see if it's worth changing it. -- To view, visit https://gerrit.osmocom.org/12377 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc3a2896b133298cbf850d68e6898300884bbce Gerrit-Change-Number: 12377 Gerrit-PatchSet: 5 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Thu, 20 Dec 2018 11:27:02 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/12377 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/12377 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc3a2896b133298cbf850d68e6898300884bbce Gerrit-Change-Number: 12377 Gerrit-PatchSet: 5 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Thu, 20 Dec 2018 11:15:35 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Max has posted comments on this change. ( https://gerrit.osmocom.org/12377 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 4: This change is ready for review. -- To view, visit https://gerrit.osmocom.org/12377 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dc3a2896b133298cbf850d68e6898300884bbce Gerrit-Change-Number: 12377 Gerrit-PatchSet: 4 Gerrit-Owner: Max Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Thu, 20 Dec 2018 10:57:41 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Max has posted comments on this change. ( https://gerrit.osmocom.org/12369 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 6: Ok, I've split API-change into separate patch from behavior fix. Hopefully this will make review easier. -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 6 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: dexter Gerrit-Comment-Date: Thu, 20 Dec 2018 10:24:48 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
dexter has posted comments on this change. ( https://gerrit.osmocom.org/12369 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 1: (1 comment) > Build Successful > > https://jenkins.osmocom.org/jenkins/job/gerrit-libosmocore/1353/ : > SUCCESS' --verified 1 --code-review 0 https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h File include/osmocom/gsm/gsm0808.h: https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h@a160 PS1, Line 160: > Is this bit then set by the user of the API? If there's users using this API, > we should modify them […] The bit is set by the API user. Probably its best to ask neels, he designed this part of the API. I just modeled the API of the handover performed message after what was already there. (I still don't get whats the problem here.) -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: dexter Gerrit-Comment-Date: Thu, 20 Dec 2018 08:02:09 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Max has posted comments on this change. ( https://gerrit.osmocom.org/12369 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 4: This change is ready for review. -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 4 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: dexter Gerrit-Comment-Date: Wed, 19 Dec 2018 18:59:20 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12369 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 1: Code-Review-1 (1 comment) https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h File include/osmocom/gsm/gsm0808.h: https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h@a160 PS1, Line 160: > When the _present flag is set to false, then the related value is not sent. Is this bit then set by the user of the API? If there's users using this API, we should modify them too. If not, then this code was wrong since this value was always false. BTW, we are breaking the API/ABI here. Was this field added after last release? If that's not the case, I see no good reason for breaking the ABI here. -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: dexter Gerrit-Comment-Date: Wed, 19 Dec 2018 17:59:16 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
dexter has posted comments on this change. ( https://gerrit.osmocom.org/12369 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 1: (2 comments) > (1 comment) https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h File include/osmocom/gsm/gsm0808.h: https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h@a160 PS1, Line 160: > AFAIK, yes. […] When the _present flag is set to false, then the related value is not sent. https://gerrit.osmocom.org/#/c/12369/1/src/gsm/gsm0808.c File src/gsm/gsm0808.c: https://gerrit.osmocom.org/#/c/12369/1/src/gsm/gsm0808.c@901 PS1, Line 901: if (params->lcls_bss_status != GSM0808_LCLS_STS_NA) I am not sure if this is a good idea. GSM0808_LCLS_STS_NA = 0xFF, so one must never forget to initialize the field when creating the message. However, in the end its a matter of taste anyway. -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: dexter Gerrit-CC: Pau Espin Pedrol Gerrit-Comment-Date: Wed, 19 Dec 2018 17:12:16 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Max has posted comments on this change. ( https://gerrit.osmocom.org/12369 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h File include/osmocom/gsm/gsm0808.h: https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h@a160 PS1, Line 160: > So those were never set to 1? then status was never sent? AFAIK, yes. @dexter? -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: dexter Gerrit-CC: Pau Espin Pedrol Gerrit-Comment-Date: Wed, 19 Dec 2018 16:57:00 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12369 ) Change subject: LCLS: don't send invalid status in HO messages .. Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h File include/osmocom/gsm/gsm0808.h: https://gerrit.osmocom.org/#/c/12369/1/include/osmocom/gsm/gsm0808.h@a160 PS1, Line 160: So those were never set to 1? then status was never sent? -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 1 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: dexter Gerrit-CC: Pau Espin Pedrol Gerrit-Comment-Date: Wed, 19 Dec 2018 16:55:33 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: LCLS: don't send invalid status in HO messages
Max has uploaded this change for review. ( https://gerrit.osmocom.org/12369 Change subject: LCLS: don't send invalid status in HO messages .. LCLS: don't send invalid status in HO messages Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b --- M include/osmocom/gsm/gsm0808.h M src/gsm/gsm0808.c 2 files changed, 2 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/69/12369/1 diff --git a/include/osmocom/gsm/gsm0808.h b/include/osmocom/gsm/gsm0808.h index 79d89e5..ccecda4 100644 --- a/include/osmocom/gsm/gsm0808.h +++ b/include/osmocom/gsm/gsm0808.h @@ -157,7 +157,6 @@ bool chosen_channel_present; uint8_t chosen_channel; - bool lcls_bss_status_present; enum gsm0808_lcls_status lcls_bss_status; /* more items are defined in the spec and may be added later */ @@ -194,7 +193,6 @@ bool speech_codec_chosen_present; struct gsm0808_speech_codec speech_codec_chosen; - bool lcls_bss_status_present; enum gsm0808_lcls_status lcls_bss_status; /* more items are defined in the spec and may be added later */ diff --git a/src/gsm/gsm0808.c b/src/gsm/gsm0808.c index 69da57d..192c0be 100644 --- a/src/gsm/gsm0808.c +++ b/src/gsm/gsm0808.c @@ -898,7 +898,7 @@ msgb_tv_put(msg, GSM0808_IE_CHOSEN_ENCR_ALG, params->chosen_encr_alg); /* LCLS-BSS-Status 3.2.2.119 */ - if (params->lcls_bss_status_present) + if (params->lcls_bss_status != GSM0808_LCLS_STS_NA) msgb_tv_put(msg, GSM0808_IE_LCLS_BSS_STATUS, params->lcls_bss_status); /* prepend header with final length */ @@ -974,7 +974,7 @@ gsm0808_enc_speech_codec(msg, >speech_codec_chosen); /* LCLS-BSS-Status 3.2.2.119 */ - if (params->lcls_bss_status_present) + if (params->lcls_bss_status != GSM0808_LCLS_STS_NA) msgb_tv_put(msg, GSM0808_IE_LCLS_BSS_STATUS, params->lcls_bss_status); /* prepend header with final length */ -- To view, visit https://gerrit.osmocom.org/12369 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib2958a8613627c4e54c004ffa3578c300ed0360b Gerrit-Change-Number: 12369 Gerrit-PatchSet: 1 Gerrit-Owner: Max
