diff mbox

openbsc[master]: gsm04_08_clear_request(): release loc with arg release=0

Message ID gerrit.1463841854964.I5bf9eb4889d32ad5e42ac7d096bf62fa3a493e20@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 21, 2016, 2:44 p.m. UTC 
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

Comments

gerrit-no-reply@lists.osmocom.org May 22, 2016, 11:16 a.m. UTC | #1 
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?
gerrit-no-reply@lists.osmocom.org May 22, 2016, 11:54 a.m. UTC | #2 
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.
gerrit-no-reply@lists.osmocom.org May 23, 2016, 4:25 p.m. UTC | #3 
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.
diff mbox

Patch

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) {