Change in libosmocore[master]: GSUP: add Class IE
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/13576 )
Change subject: GSUP: add Class IE
..
Patch Set 3:
(2 comments)
https://gerrit.osmocom.org/#/c/13576/3/src/gsm/gsup.c
File src/gsm/gsup.c:
https://gerrit.osmocom.org/#/c/13576/3/src/gsm/gsup.c@481
PS3, Line 481: osmo_decode_big_endian
> It's just one byte, no need for osmo_decode_big_endian() here.
was just copying code I saw elsewhere; you are right though, I think I copied
uint32_t code without noticing.
https://gerrit.osmocom.org/#/c/13576/3/src/gsm/gsup.c@739
PS3, Line 739: {}
> Common style: { 0, NULL }
I always use {}, and have been doing that all over the place.
--
To view, visit https://gerrit.osmocom.org/13576
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: Ic397a9f2c4a7224e47cab944c72e75ca5592efef
Gerrit-Change-Number: 13576
Gerrit-PatchSet: 3
Gerrit-Owner: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Vadim Yanitskiy
Gerrit-Comment-Date: Thu, 11 Apr 2019 23:33:52 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in libosmocore[master]: GSUP: add Class IE
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13576 ) Change subject: GSUP: add Class IE .. Patch Set 3: (1 comment) https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h File include/osmocom/gsm/gsup.h: https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h@309 PS3, Line 309: enum osmo_gsup_classclass; Actually in this case I notice that the word 'class' alone is a keyword in C++, so it was in fact a poor choice for a variable / member name. So ... might have to format-patch-sed-git-am again This here would have been a saner place to argue against the name than above :P I wish I had stayed firm with just using "Kind" instead, what's the use of changing synonyms for synonyms. -- To view, visit https://gerrit.osmocom.org/13576 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: Ic397a9f2c4a7224e47cab944c72e75ca5592efef Gerrit-Change-Number: 13576 Gerrit-PatchSet: 3 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Thu, 11 Apr 2019 23:30:27 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: GSUP: add Class IE
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13576 ) Change subject: GSUP: add Class IE .. Patch Set 3: (2 comments) I appreciate the review, but in this case... https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h File include/osmocom/gsm/gsup.h: https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h@71 PS3, Line 71: OSMO_GSUP_CLASS_IE > Looking at this IE, the reader may ask: class of what? […] It is actually rather absurd, in the face of HUGE GIGANTIC patches waiting for review, we are seriously nitpicking about "Kind" vs "Class" vs "Message Class". This is Bikeshed at its best, if I may say so myself ;) "Message Class" is saying exactly the same thing but with more words; what part of GSUP is not a Message? I already once edited all those patches from "KIND" to "CLASS", here, in osmo-hlr and osmo-msc, across entire branch histories; with git format-patch, sed and git am, taking great care to not accidentally modify other sentences containing the word "kind" in unrelated contexts. IMHO that's no sane way to spend my time. https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h@241 PS3, Line 241: OSMO_GSUP_CLASS_ARRAYSIZE > Which array? […] Please see the comment just above it. It explicitly serves as size for an array, hence it is called ARRAYSIZE. The other may be an End Marker, which it actually isn't: it never is actually used as a marker anywhere. The only place it is used is gsup_test.c for a validity check. Apologies, but I'd rather not adopt a bad name choice if you don't force me. -- To view, visit https://gerrit.osmocom.org/13576 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: Ic397a9f2c4a7224e47cab944c72e75ca5592efef Gerrit-Change-Number: 13576 Gerrit-PatchSet: 3 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Thu, 11 Apr 2019 23:24:10 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: GSUP: add Class IE
Vadim Yanitskiy has posted comments on this change. (
https://gerrit.osmocom.org/13576 )
Change subject: GSUP: add Class IE
..
Patch Set 3:
(2 comments)
https://gerrit.osmocom.org/#/c/13576/3/src/gsm/gsup.c
File src/gsm/gsup.c:
https://gerrit.osmocom.org/#/c/13576/3/src/gsm/gsup.c@481
PS3, Line 481: osmo_decode_big_endian
It's just one byte, no need for osmo_decode_big_endian() here.
https://gerrit.osmocom.org/#/c/13576/3/src/gsm/gsup.c@739
PS3, Line 739: {}
Common style: { 0, NULL }
--
To view, visit https://gerrit.osmocom.org/13576
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: Ic397a9f2c4a7224e47cab944c72e75ca5592efef
Gerrit-Change-Number: 13576
Gerrit-PatchSet: 3
Gerrit-Owner: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Vadim Yanitskiy
Gerrit-Comment-Date: Thu, 11 Apr 2019 08:57:01 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in libosmocore[master]: GSUP: add Class IE
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/13576 ) Change subject: GSUP: add Class IE .. Patch Set 3: Code-Review-1 (3 comments) https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h File include/osmocom/gsm/gsup.h: https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h@71 PS3, Line 71: OSMO_GSUP_CLASS_IE Looking at this IE, the reader may ask: class of what? How about 'OSMO_GSUP_MSG_CLASS_IE' or 'OSMO_GSUP_MESSAGE_CLASS_IE'? https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h@233 PS3, Line 233: osmo_gsup_class Same comment regarding the naming here. https://gerrit.osmocom.org/#/c/13576/3/include/osmocom/gsm/gsup.h@241 PS3, Line 241: OSMO_GSUP_CLASS_ARRAYSIZE Which array? Enum osmo_gsup_iei has '_OSMO_GSUP_IEI_END_MARKER'. I think we should keep the naming style consistent. -- To view, visit https://gerrit.osmocom.org/13576 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: Ic397a9f2c4a7224e47cab944c72e75ca5592efef Gerrit-Change-Number: 13576 Gerrit-PatchSet: 3 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Thu, 11 Apr 2019 08:54:47 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in libosmocore[master]: GSUP: add Class IE
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/13576 ) Change subject: GSUP: add Class IE .. Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/13576 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: Ic397a9f2c4a7224e47cab944c72e75ca5592efef Gerrit-Change-Number: 13576 Gerrit-PatchSet: 3 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Comment-Date: Thu, 11 Apr 2019 06:00:10 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmocore[master]: GSUP: add Class IE
Hello Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/13576 to look at the new patch set (#2). Change subject: GSUP: add Class IE .. GSUP: add Class IE osmo-msc and osmo-hlr have distinct subsystems handling incoming GSUP messages. So far we decide entirely by message type which code path should handle a GSUP message. Thus no GSUP message type may be re-used across subsystems. If we add a GSUP message to indicate a routing error, it would have to be a distinct message type for subscriber management, another one for SMS, another one for USSD... To allow introducing common message types, introduce a GSUP Class IE. In the presence of this IE, GSUP handlers can trivially direct a received message to the right code path. If it is missing, handlers can fall back to the previous switch(message_type) method. Change-Id: Ic397a9f2c4a7224e47cab944c72e75ca5592efef --- M include/osmocom/gsm/gsup.h M src/gsm/gsup.c M src/gsm/libosmogsm.map M tests/gsup/gsup_test.c M tests/gsup/gsup_test.err 5 files changed, 47 insertions(+), 5 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/76/13576/2 -- To view, visit https://gerrit.osmocom.org/13576 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic397a9f2c4a7224e47cab944c72e75ca5592efef Gerrit-Change-Number: 13576 Gerrit-PatchSet: 2 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102)
