Change in libosmocore[master]: gsm48: move to hex TMSI representation
diff mbox

Message ID gerrit.1463123814609.Ifd25365bfa3b4ee95b16979740c3229948ce17f2@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 13, 2016, 7:16 a.m. UTC
From Vadim Yanitskiy <axilirator@gmail.com>:

Vadim Yanitskiy has uploaded a new change for review.

  https://gerrit.osmocom.org/57

Change subject: gsm48: move to hex TMSI representation
......................................................................

gsm48: move to hex TMSI representation

Previously, we traditionally displayed a TMSI in its integer
representation, which is quite unusual in the telecom world.
A TMSI is normally printed as a series of 8 hex digits.

Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2
---
M src/gsm/gsm48.c
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/57/57/1

Comments

gerrit-no-reply@lists.osmocom.org May 14, 2016, 10:15 a.m. UTC | #1
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: gsm48: move to hex TMSI representation
......................................................................


Patch Set 1: Code-Review+2

(2 comments)

https://gerrit.osmocom.org/#/c/57/1/src/gsm/gsm48.c
File src/gsm/gsm48.c:

PS1, Line 461:  
It is not a "reverse" anymore. Should the comment be updated? Specially with gsm48_generate_mid_from_tmsi just taking the uint32?


PS1, Line 465: string
Why do you prefix it with 0x here?  In the commit message I would like to have a small statement about the minimum size of the "string" not having.

Who consumes the result and does the represntation matter?
gerrit-no-reply@lists.osmocom.org May 14, 2016, 10:15 a.m. UTC | #2
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: gsm48: move to hex TMSI representation
......................................................................


Patch Set 1: Code-Review+1
gerrit-no-reply@lists.osmocom.org May 14, 2016, 4:02 p.m. UTC | #3
From Vadim Yanitskiy <axilirator@gmail.com>:

Vadim Yanitskiy has posted comments on this change.

Change subject: gsm48: move to hex TMSI representation
......................................................................


Patch Set 1:

(2 comments)

> (2 comments)

https://gerrit.osmocom.org/#/c/57/1/src/gsm/gsm48.c
File src/gsm/gsm48.c:

PS1, Line 461:  
> It is not a "reverse" anymore. Should the comment be updated? Specially wit
I think we can simply delete this comment.


PS1, Line 465: string
> Why do you prefix it with 0x here?  In the commit message I would like to h
Because I saw that the 0x%08x format also used in other Osmocom projects, for example in OsmocomBB. Well, the minimum ant the maximum size of the string is always constant and equal to 2 + 8 = 10 (without '\0').

It is mostly consumed by user, so I think we should use the most comfortable for reading format.

Would you prefer to avoid the '0x' prefix in this case?
Also, what about using '%08X' instead of '%08x'?
gerrit-no-reply@lists.osmocom.org May 15, 2016, 9:23 p.m. UTC | #4
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: gsm48: move to hex TMSI representation
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

https://gerrit.osmocom.org/#/c/57/1/src/gsm/gsm48.c
File src/gsm/gsm48.c:

PS1, Line 465: string
> Because I saw that the 0x%08x format also used in other Osmocom projects, f
I think the general representation in the industry would be all-uppercase and without 0x, so "%08X". However, as we used to have integer representation in OpenBSC so far, it might be a good idea to give the '0x' as a hint to the user, so he knows this version of the log statement/vty output is in hex, as opposed to earlier versions.
gerrit-no-reply@lists.osmocom.org May 17, 2016, 4:15 p.m. UTC | #5
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: gsm48: move to hex TMSI representation
......................................................................


Patch Set 1:

Dear Vadim,

could you address the review feedback and then use git push gerrit.. to upload a new patchset with the changes requested? As you and Harald proposed we use 0x
gerrit-no-reply@lists.osmocom.org May 20, 2016, 3:44 p.m. UTC | #6
Patch Set 2: Code-Review+1
gerrit-no-reply@lists.osmocom.org May 20, 2016, 4:26 p.m. UTC | #7
Patch Set 2: Code-Review+2

Patch
diff mbox

diff --git a/src/gsm/gsm48.c b/src/gsm/gsm48.c
index 8a46f76..74a1e99 100644
--- a/src/gsm/gsm48.c
+++ b/src/gsm/gsm48.c
@@ -462,7 +462,7 @@ 
 		if (mi_len == GSM48_TMSI_LEN && mi[0] == (0xf0 | GSM_MI_TYPE_TMSI)) {
 			memcpy(&tmsi, &mi[1], 4);
 			tmsi = ntohl(tmsi);
-			return snprintf(string, str_len, "%u", tmsi);
+			return snprintf(string, str_len, "0x%08x", tmsi);
 		}
 		break;
 	case GSM_MI_TYPE_IMSI: