Change in libosmocore[master]: gsm48: move to hex TMSI representation
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 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: -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com>
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? -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
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 -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: No
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'? -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-HasComments: Yes
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.
-- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-HasComments: Yes
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 -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-HasComments: No
Hello Harald Welte, Jenkins Builder, Holger Freyther, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/57 to look at the new patch set (#2). 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. Review at https://gerrit.osmocom.org/57/ Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 --- M src/gsm/gsm48.c 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/57/57/2 diff --git a/src/gsm/gsm48.c b/src/gsm/gsm48.c index 8a46f76..d0e050b 100644 --- a/src/gsm/gsm48.c +++ b/src/gsm/gsm48.c @@ -458,11 +458,10 @@ case GSM_MI_TYPE_NONE: break; case GSM_MI_TYPE_TMSI: - /* Table 10.5.4.3, reverse generate_mid_from_tmsi */ 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: -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 2 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com>
Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 2 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-HasComments: No
Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 2 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-HasComments: No
Holger Freyther has submitted this change and it was merged. 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. Review at https://gerrit.osmocom.org/57/ Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Reviewed-on: https://gerrit.osmocom.org/57 Tested-by: Jenkins Builder Reviewed-by: Harald Welte <laforge@gnumonks.org> Reviewed-by: Holger Freyther <holger@freyther.de> --- M src/gsm/gsm48.c 1 file changed, 1 insertion(+), 2 deletions(-) Approvals: Harald Welte: Looks good to me, but someone else must approve Jenkins Builder: Verified Holger Freyther: Looks good to me, approved diff --git a/src/gsm/gsm48.c b/src/gsm/gsm48.c index 8a46f76..d0e050b 100644 --- a/src/gsm/gsm48.c +++ b/src/gsm/gsm48.c @@ -458,11 +458,10 @@ case GSM_MI_TYPE_NONE: break; case GSM_MI_TYPE_TMSI: - /* Table 10.5.4.3, reverse generate_mid_from_tmsi */ 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: -- To view, visit https://gerrit.osmocom.org/57 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ifd25365bfa3b4ee95b16979740c3229948ce17f2 Gerrit-PatchSet: 3 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com>
participants (4)
-
gerrit-no-reply＠lists.osmocom.org -
Harald Welte
-
Holger Freyther
-
Vadim Yanitskiy