Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11728 )
Change subject: gsm0808: add message generator for BSSMAP HANDOVER PERFORMED ...................................................................... Patch Set 6: Code-Review+2 (3 comments) https://gerrit.osmocom.org/#/c/11728/6/include/osmocom/gsm/gsm0808.h File include/osmocom/gsm/gsm0808.h: https://gerrit.osmocom.org/#/c/11728/6/include/osmocom/gsm/gsm0808.h@188 PS6, Line 188: bool lcls_bss_status_present; > You can use GSM0808_LCLS_STS_NA - no need in separate bool variable. my problem with that is that GSM0808_LCLS_STS_NA is != 0. So if I create a gsm0808_handover_performed struct, this would be the only item where I need to set something in order to disable the IE. So I'm -1 on using the STS_NA and +1 the current patch version. Also re: adding struct osmo_lcls suggested earlier, that would be a bit of a layer violation. This function should only encode specific values, not query a status struct and so forth. So again IMHO this patch version is better. https://gerrit.osmocom.org/#/c/11728/6/src/gsm/gsm0808.c File src/gsm/gsm0808.c: https://gerrit.osmocom.org/#/c/11728/6/src/gsm/gsm0808.c@913 PS6, Line 913: msgb_tlv_put(msg, GSM0808_IE_CAUSE, gsm0808_cause_ext(params->cause) ? 2 : 1, (const uint8_t *)¶ms->cause); it was suggested to look at gsm0808_create_cipher_reject_ext(), but clearly that is about an unrelated IE, the implementation also just "manually" encodes the length and can obviously not be re-used here. Hence I would prefer to merge this as it is, and possibly unify the length finnacky stuff as a general-purpose function in a separate patch, which would then also include some .c regression test. https://gerrit.osmocom.org/#/c/11728/6/src/gsm/gsm0808.c@935 PS6, Line 935: if (params->lcls_bss_status_present) > Same here - check for GSM0808_LCLS_STS_NA and add if it differs. (same, I like the current patch version better) -- To view, visit https://gerrit.osmocom.org/11728 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: I825106858bd89afc9837811b8fed2e8accc82441 Gerrit-Change-Number: 11728 Gerrit-PatchSet: 6 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: dexter <[email protected]> Gerrit-CC: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Fri, 30 Nov 2018 15:13:20 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
