[PATCH] openbsc[master]: gsm04_08_clear_request(): release loc with arg release=0
Review at https://gerrit.osmocom.org/93 gsm04_08_clear_request(): release loc with arg release=0 In gsm04_08_clear_request(), in_release == 1 anyway and msc_release_connection() would exit immediately without any effect. Don't confuse the reader by passing release=1 arg. Change-Id: I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20 --- M openbsc/src/libmsc/gsm_04_08.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/93/93/1 diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index f02f784..f1b95c1 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -384,7 +384,7 @@ * Cancel any outstanding location updating request * operation taking place on the subscriber connection. */ - release_loc_updating_req(conn, 1); + release_loc_updating_req(conn, 0); /* We might need to cancel the paging response or such. */ if (conn->sec_operation && conn->sec_operation->cb) { -- To view, visit https://gerrit.osmocom.org/93 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20 Gerrit-PatchSet: 1 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de>
Patch Set 2: I think what we can argue about is. Either way you need to wonder how the calls deal in case of the release. * Either you know that release_loc_updating_req can be asked not to msc_release_connection * Or you know that because you set in_relase = 1, msc_release_connection will be called but that it is a no-op. So do you really think that doing it this way is more readable/more obvious? -- To view, visit https://gerrit.osmocom.org/93 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: No
Patch Set 2: I know that in_release==1 so msc_release_connection() is a nop. It is really weird to set a flag that disables a function completely and then call that function anyway. I find that confusing, but if you don't agree we can drop this change. -- To view, visit https://gerrit.osmocom.org/93 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> Gerrit-HasComments: No
Patch Set 2: Code-Review+2 I have no preference here. I just think we exchange one requirement of knowledge about the code with another one. -- To view, visit https://gerrit.osmocom.org/93 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> Gerrit-HasComments: No
Holger Freyther has submitted this change and it was merged. Change subject: gsm04_08_clear_request(): release loc with arg release=0 ...................................................................... gsm04_08_clear_request(): release loc with arg release=0 In gsm04_08_clear_request(), in_release == 1 anyway and msc_release_connection() would exit immediately without any effect. Don't confuse the reader by passing release=1 arg. Change-Id: I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20 Reviewed-on: https://gerrit.osmocom.org/93 Reviewed-by: Holger Freyther <holger@freyther.de> Tested-by: Jenkins Builder --- M openbsc/src/libmsc/gsm_04_08.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Jenkins Builder: Verified Holger Freyther: Looks good to me, approved diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 68cc906..74da34b 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -384,7 +384,7 @@ * Cancel any outstanding location updating request * operation taking place on the subscriber connection. */ - release_loc_updating_req(conn, 1); + release_loc_updating_req(conn, 0); /* We might need to cancel the paging response or such. */ if (conn->sec_operation && conn->sec_operation->cb) { -- To view, visit https://gerrit.osmocom.org/93 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20 Gerrit-PatchSet: 4 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de>
participants (2)
-
Holger Freyther -
Neels Hofmeyr