Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11989 ) Change subject: gsupclient: add osmo_gsup_msg_enc_send() .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/11989 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f Gerrit-Change-Number: 11989 Gerrit-PatchSet: 5 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Mon, 17 Dec 2018 11:28:32 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Hello Stefan Sperling, Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11989 to look at the new patch set (#5). Change subject: gsupclient: add osmo_gsup_msg_enc_send() .. gsupclient: add osmo_gsup_msg_enc_send() Several parts of OsmoMSC (e.g. GSM 04.11, 09.11, etc.) are dealing with GSUP message encoding and sending towards OsmoHLR. In order to avoid code duplication, let's have a shared function here. Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f --- M include/osmocom/gsupclient/gsup_client.h M src/gsupclient/gsup_client.c 2 files changed, 39 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/89/11989/5 -- To view, visit https://gerrit.osmocom.org/11989 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f Gerrit-Change-Number: 11989 Gerrit-PatchSet: 5 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: Vadim Yanitskiy
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Vadim Yanitskiy has posted comments on this change. (
https://gerrit.osmocom.org/11989 )
Change subject: gsupclient: add osmo_gsup_msg_enc_send()
..
Patch Set 4:
(3 comments)
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c
File src/gsupclient/gsup_client.c:
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c@366
PS4, Line 366: * \returns 0 in case of success, otherwise errno
> Errno is positive and we return negative I believe - better state that
> explicitly.
Ok.
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c@381
PS4, Line 381: if (rc) {
> to make reading code easier.
Easier? I don't think the current version is complicated.
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c@387
PS4, Line 387: if (rc) {
> And here too.
And here too. It's a common practice to expect 0 and nothing else.
--
To view, visit https://gerrit.osmocom.org/11989
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f
Gerrit-Change-Number: 11989
Gerrit-PatchSet: 4
Gerrit-Owner: Vadim Yanitskiy
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Stefan Sperling
Gerrit-Reviewer: Vadim Yanitskiy
Gerrit-Comment-Date: Fri, 14 Dec 2018 13:21:10 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Max has posted comments on this change. ( https://gerrit.osmocom.org/11989 )
Change subject: gsupclient: add osmo_gsup_msg_enc_send()
..
Patch Set 4: Code-Review-1
(3 comments)
Sorry, haven't noticed those earlier.
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c
File src/gsupclient/gsup_client.c:
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c@366
PS4, Line 366: * \returns 0 in case of success, otherwise errno
Errno is positive and we return negative I believe - better state that
explicitly.
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c@381
PS4, Line 381: if (rc) {
I think it's better to explicitly check rc < 0 in here to make reading code
easier.
https://gerrit.osmocom.org/#/c/11989/4/src/gsupclient/gsup_client.c@387
PS4, Line 387: if (rc) {
And here too.
--
To view, visit https://gerrit.osmocom.org/11989
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f
Gerrit-Change-Number: 11989
Gerrit-PatchSet: 4
Gerrit-Owner: Vadim Yanitskiy
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Stefan Sperling
Gerrit-Comment-Date: Fri, 14 Dec 2018 13:07:38 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Hello Stefan Sperling, Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11989 to look at the new patch set (#4). Change subject: gsupclient: add osmo_gsup_msg_enc_send() .. gsupclient: add osmo_gsup_msg_enc_send() Several parts of OsmoMSC (e.g. GSM 04.11, 09.11, etc.) are dealing with GSUP message encoding and sending towards OsmoHLR. In order to avoid code duplication, let's have a shared function here. Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f --- M include/osmocom/gsupclient/gsup_client.h M src/gsupclient/gsup_client.c 2 files changed, 39 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/89/11989/4 -- To view, visit https://gerrit.osmocom.org/11989 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f Gerrit-Change-Number: 11989 Gerrit-PatchSet: 4 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Max has posted comments on this change. ( https://gerrit.osmocom.org/11989 ) Change subject: gsupclient: add osmo_gsup_msg_enc_send() .. Patch Set 3: Code-Review-1 (2 comments) Please double check. https://gerrit.osmocom.org/#/c/11989/3/src/gsupclient/gsup_client.c File src/gsupclient/gsup_client.c: https://gerrit.osmocom.org/#/c/11989/3/src/gsupclient/gsup_client.c@369 PS3, Line 369: struct osmo_gsup_message *gsup_msg) Why this isn't const? What modifies it? https://gerrit.osmocom.org/#/c/11989/3/src/gsupclient/gsup_client.c@380 PS3, Line 380: rc = osmo_gsup_encode(gsup_msgb, gsup_msg); gsup_msg is const in here according to osmo_gsup_encode() type signature. -- To view, visit https://gerrit.osmocom.org/11989 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f Gerrit-Change-Number: 11989 Gerrit-PatchSet: 3 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Fri, 14 Dec 2018 12:51:12 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11989 ) Change subject: gsupclient: add osmo_gsup_msg_enc_send() .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/11989 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f Gerrit-Change-Number: 11989 Gerrit-PatchSet: 3 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Fri, 14 Dec 2018 12:16:28 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send()
Hello Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11989 to look at the new patch set (#3). Change subject: gsupclient: add osmo_gsup_msg_enc_send() .. gsupclient: add osmo_gsup_msg_enc_send() Several parts of OsmoMSC (e.g. GSM 04.11, 09.11, etc.) are dealing with GSUP message encoding and sending towards OsmoHLR. In order to avoid code duplication, let's have a shared function here. Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f --- M include/osmocom/gsupclient/gsup_client.h M src/gsupclient/gsup_client.c 2 files changed, 39 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/89/11989/3 -- To view, visit https://gerrit.osmocom.org/11989 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f Gerrit-Change-Number: 11989 Gerrit-PatchSet: 3 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send() helper
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/11989 )
Change subject: gsupclient: add osmo_gsup_msg_enc_send() helper
..
Patch Set 2: Code-Review-1
(6 comments)
https://gerrit.osmocom.org/#/c/11989/2//COMMIT_MSG
Commit Message:
https://gerrit.osmocom.org/#/c/11989/2//COMMIT_MSG@7
PS2, Line 7: gsupclient: add osmo_gsup_msg_enc_send() helper
("helper" triggers a rant from me: it is a word like "framework" or "thing".
All functions except main() are helpers.)
https://gerrit.osmocom.org/#/c/11989/2//COMMIT_MSG@10
PS2, Line 10: messages using a given abstract 'osmo_gsup_message' structure.
what the function does should be documented at the new function. the commit log
is more about understanding why and how the change came to be.
And that is exactly what is missing here. All that I can tell is that you are
adding dead code??
https://gerrit.osmocom.org/#/c/11989/2/src/gsupclient/gsup_client.c
File src/gsupclient/gsup_client.c:
https://gerrit.osmocom.org/#/c/11989/2/src/gsupclient/gsup_client.c@342
PS2, Line 342: /* Helper for encoding and sending GSUP messages */
/* Encode and send a GSUP message. */
Though, that part is rather obvious from the function name, while e.g. the
return value is not explained.
https://gerrit.osmocom.org/#/c/11989/2/src/gsupclient/gsup_client.c@349
PS2, Line 349: /* Allocate GSUP message buffer */
omit comments that merely restate the code
https://gerrit.osmocom.org/#/c/11989/2/src/gsupclient/gsup_client.c@356
PS2, Line 356: /* Encode GSUP message */
ditto
https://gerrit.osmocom.org/#/c/11989/2/src/gsupclient/gsup_client.c@363
PS2, Line 363: /* Finally send */
Do you know these API docs that go like:
set_value(): Helper to set value.
osmo_gsup_encode(): Encode an OsmoGSUP.
osmo_gsup_client_send(): Send an OsmoGSUP client.
;)
--
To view, visit https://gerrit.osmocom.org/11989
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f
Gerrit-Change-Number: 11989
Gerrit-PatchSet: 2
Gerrit-Owner: Vadim Yanitskiy
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Comment-Date: Thu, 06 Dec 2018 01:47:41 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in osmo-hlr[master]: gsupclient: add osmo_gsup_msg_enc_send() helper
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/11989 ) Change subject: gsupclient: add osmo_gsup_msg_enc_send() helper .. Set Ready For Review -- To view, visit https://gerrit.osmocom.org/11989 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0589ff27933e9bca2bcf93b8259004935778db8f Gerrit-Change-Number: 11989 Gerrit-PatchSet: 1 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Comment-Date: Wed, 28 Nov 2018 19:21:05 + Gerrit-HasComments: No Gerrit-HasLabels: No
