September 2023 - gerrit-log - lists.osmocom.org

[M] Change in osmo-bsc[master]: abis_nm: separate parsing of osmo-bts features into a function

by fixeria

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/34322?usp=email ) Change subject: abis_nm: separate parsing of osmo-bts features into a function ...................................................................... abis_nm: separate parsing of osmo-bts features into a function This commit prepares for adding handling of additional attributes. The parse_attr_resp_info_attr() is already quite complex, so take a chance to simplify it a bit. Change-Id: Ia5919a8311cd6a7fc16d02d2196276881e96f4c5 Related: OS#4505 --- M src/osmo-bsc/abis_nm.c 1 file changed, 62 insertions(+), 33 deletions(-) Approvals: Jenkins Builder: Verified pespin: Looks good to me, approved diff --git a/src/osmo-bsc/abis_nm.c b/src/osmo-bsc/abis_nm.c index 5aaf069..aa6a3a5 100644 --- a/src/osmo-bsc/abis_nm.c +++ b/src/osmo-bsc/abis_nm.c @@ -560,6 +560,46 @@ return ari + num_unreported + 1; /* we have to account for 1st byte with number of unreported attributes */ } + +/* Parse Attribute Response Info content for 3GPP TS 52.021 §9.4.30 Manufacturer Id */ +static void parse_osmo_bts_features(struct gsm_bts *bts, + const uint8_t *data, uint16_t data_len) +{ + /* log potential BTS feature vector overflow */ + if (data_len > sizeof(bts->_features_data)) { + LOGPMO(&bts->mo, DNM, LOGL_NOTICE, + "Get Attributes Response: feature vector is truncated " + "(from %u to %zu bytes)\n", data_len, sizeof(bts->_features_data)); + data_len = sizeof(bts->_features_data); + } + + /* check that max. expected BTS attribute is above given feature vector length */ + if (data_len > OSMO_BYTES_FOR_BITS(_NUM_BTS_FEAT)) { + LOGPMO(&bts->mo, DNM, LOGL_NOTICE, + "Get Attributes Response: reported unexpectedly long (%u bytes) " + "feature vector - most likely it was compiled against newer BSC headers. " + "Consider upgrading your BSC to later version.\n", data_len); + } + + memcpy(bts->_features_data, data, data_len); + bts->features_known = true; + + /* Log each BTS feature in the reported vector */ + for (unsigned int i = 0; i < data_len * 8; i++) { + if (!osmo_bts_has_feature(&bts->features, i)) + continue; + + if (i >= _NUM_BTS_FEAT) { + LOGPMO(&bts->mo, DNM, LOGL_NOTICE, + "Get Attributes Response: unknown feature 0x%02x is supported\n", i); + } else { + LOGPMO(&bts->mo, DNM, LOGL_NOTICE, + "Get Attributes Response: feature '%s' is supported\n", + osmo_bts_features_name(i)); + } + } +} + /* Handle 3GPP TS 52.021 §8.11.3 Get Attribute Response (with nanoBTS specific attribute formatting) */ static int parse_attr_resp_info_attr(struct gsm_bts *bts, const struct gsm_bts_trx *trx, struct abis_om_fom_hdr *foh, struct tlv_parsed *tp) { @@ -572,40 +612,15 @@ char unit_id[40]; struct abis_nm_sw_desc sw_descr[MAX_BTS_ATTR]; - /* Parse Attribute Response Info content for 3GPP TS 52.021 §9.4.30 Manufacturer Id */ - if (bts->type == GSM_BTS_TYPE_OSMOBTS && TLVP_PRES_LEN(tp, NM_ATT_MANUF_ID, 2)) { - len = TLVP_LEN(tp, NM_ATT_MANUF_ID); - - /* log potential BTS feature vector overflow */ - if (len > sizeof(bts->_features_data)) { - LOGPMO(&bts->mo, DNM, LOGL_NOTICE, "Get Attributes Response: feature vector is truncated " - "(from %u to %zu bytes)\n", len, sizeof(bts->_features_data)); - len = sizeof(bts->_features_data); + switch (bts->type) { + case GSM_BTS_TYPE_OSMOBTS: + if (TLVP_PRES_LEN(tp, NM_ATT_MANUF_ID, 2)) { + parse_osmo_bts_features(bts, TLVP_VAL(tp, NM_ATT_MANUF_ID), + TLVP_LEN(tp, NM_ATT_MANUF_ID)); } - - /* check that max. expected BTS attribute is above given feature vector length */ - if (len > OSMO_BYTES_FOR_BITS(_NUM_BTS_FEAT)) { - LOGPMO(&bts->mo, DNM, LOGL_NOTICE, "Get Attributes Response: reported unexpectedly long (%u bytes) " - "feature vector - most likely it was compiled against newer BSC headers. " - "Consider upgrading your BSC to later version.\n", len); - } - - memcpy(bts->_features_data, TLVP_VAL(tp, NM_ATT_MANUF_ID), len); - bts->features_known = true; - - /* Log each BTS feature in the reported vector */ - for (i = 0; i < len * 8; i++) { - if (!osmo_bts_has_feature(&bts->features, i)) - continue; - - if (i >= _NUM_BTS_FEAT) - LOGPMO(&bts->mo, DNM, LOGL_NOTICE, "Get Attributes Response: unknown feature 0x%02x is" - " supported\n", i); - else - LOGPMO(&bts->mo, DNM, LOGL_NOTICE, "Get Attributes Response: feature '%s' is" - " supported\n", osmo_bts_features_name(i)); - } - + break; + default: + break; } /* Parse Attribute Response Info content for 3GPP TS 52.021 §9.4.28 Manufacturer Dependent State */ -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/34322?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Ia5919a8311cd6a7fc16d02d2196276881e96f4c5 Gerrit-Change-Number: 34322 Gerrit-PatchSet: 4 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 year, 9 months

1
0
0 0

[S] Change in osmo-bsc[master]: nm_{bb_transc,bts}_fsm: rework sending of Get Attributes

by fixeria

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/34354?usp=email ) Change subject: nm_{bb_transc,bts}_fsm: rework sending of Get Attributes ...................................................................... nm_{bb_transc,bts}_fsm: rework sending of Get Attributes * Make it easier to append the attributes conditionally. * Remove irrelevant comments about the order of attributes. * Request NM_ATT_IPACC_SUPP_FEATURES from Abis/IP models only. Change-Id: Ice5bddd51481a3ef9fcffd77a4c292ea211bf179 Related: OS#4505 --- M src/osmo-bsc/nm_bb_transc_fsm.c M src/osmo-bsc/nm_bts_fsm.c 2 files changed, 36 insertions(+), 12 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/osmo-bsc/nm_bb_transc_fsm.c b/src/osmo-bsc/nm_bb_transc_fsm.c index c70904b..4b5e2b2 100644 --- a/src/osmo-bsc/nm_bb_transc_fsm.c +++ b/src/osmo-bsc/nm_bb_transc_fsm.c @@ -111,13 +111,18 @@ /* Request TRX-level attributes */ if (!bb_transc->mo.get_attr_sent && !bb_transc->mo.get_attr_rep_received) { - bb_transc->mo.get_attr_sent = true; - /* N. B: we rely on attribute order when parsing response in abis_nm_rx_get_attr_resp() */ - const uint8_t trx_attr[] = { NM_ATT_MANUF_STATE, NM_ATT_SW_CONFIG, NM_ATT_IPACC_SUPP_FEATURES }; - /* we should not request more attributes than we're ready to handle */ - OSMO_ASSERT(sizeof(trx_attr) < MAX_BTS_ATTR); + uint8_t attr_buf[MAX_BTS_ATTR]; + uint8_t *ptr = &attr_buf[0]; + + *(ptr++) = NM_ATT_MANUF_STATE; + *(ptr++) = NM_ATT_SW_CONFIG; + if (is_ipa_abisip_bts(trx->bts)) + *(ptr++) = NM_ATT_IPACC_SUPP_FEATURES; + + OSMO_ASSERT((ptr - attr_buf) <= sizeof(attr_buf)); abis_nm_get_attr(trx->bts, NM_OC_BASEB_TRANSC, 0, trx->nr, 0xff, - trx_attr, sizeof(trx_attr)); + &attr_buf[0], (ptr - attr_buf)); + bb_transc->mo.get_attr_sent = true; } if (bb_transc->mo.get_attr_rep_received && diff --git a/src/osmo-bsc/nm_bts_fsm.c b/src/osmo-bsc/nm_bts_fsm.c index e601e18..a9b6604 100644 --- a/src/osmo-bsc/nm_bts_fsm.c +++ b/src/osmo-bsc/nm_bts_fsm.c @@ -98,13 +98,18 @@ /* Request generic BTS-level attributes */ if (!bts->mo.get_attr_sent && !bts->mo.get_attr_rep_received) { - bts->mo.get_attr_sent = true; - /* N. B: we rely on attribute order when parsing response in abis_nm_rx_get_attr_resp() */ - const uint8_t bts_attr[] = { NM_ATT_MANUF_ID, NM_ATT_SW_CONFIG, NM_ATT_IPACC_SUPP_FEATURES}; - /* we should not request more attributes than we're ready to handle */ - OSMO_ASSERT(sizeof(bts_attr) < MAX_BTS_ATTR); + uint8_t attr_buf[MAX_BTS_ATTR]; + uint8_t *ptr = &attr_buf[0]; + + *(ptr++) = NM_ATT_MANUF_ID; + *(ptr++) = NM_ATT_SW_CONFIG; + if (is_ipa_abisip_bts(bts)) + *(ptr++) = NM_ATT_IPACC_SUPP_FEATURES; + + OSMO_ASSERT((ptr - attr_buf) <= sizeof(attr_buf)); abis_nm_get_attr(bts, NM_OC_BTS, 0, 0xff, 0xff, - bts_attr, sizeof(bts_attr)); + &attr_buf[0], (ptr - attr_buf)); + bts->mo.get_attr_sent = true; } if (bts->mo.get_attr_rep_received && -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/34354?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Ice5bddd51481a3ef9fcffd77a4c292ea211bf179 Gerrit-Change-Number: 34354 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 year, 9 months

1
0
0 0

[S] Change in osmo-bsc[master]: abis_nm: get rid of MAX_BTS_ATTR

by fixeria

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/34355?usp=email ) Change subject: abis_nm: get rid of MAX_BTS_ATTR ...................................................................... abis_nm: get rid of MAX_BTS_ATTR This is a partial revert of commit [1], which defined a limit on the number of attributes and SW Description IEs as a constant and added a spec. reference. The problem is that there is no such limit in the referenced 3GPP TS 52.021. The attributes and SW Description IEs are using TL16V encoding, so there can be as many as the Length part can represent. It's actually the limitation of our side, since we allocate a buffer of fixed size on the stack for parsing. * Remove the MAX_BTS_ATTR and confusing spec. reference. * For the SW Description IEs, define SW_DESCR_MAX locally. * For the attributes, define the buffer size in place. Change-Id: Idd8b971d32cf0f7a910a664d921e644b7c32d831 Related: [1] 1ebf23b7fe "Prepare for BTS attribute reporting via OML" Related: OS#4505 --- M include/osmocom/bsc/abis_nm.h M src/osmo-bsc/abis_nm.c M src/osmo-bsc/nm_bb_transc_fsm.c M src/osmo-bsc/nm_bts_fsm.c 4 files changed, 30 insertions(+), 7 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/include/osmocom/bsc/abis_nm.h b/include/osmocom/bsc/abis_nm.h index bfafa63..7f1ec74 100644 --- a/include/osmocom/bsc/abis_nm.h +++ b/include/osmocom/bsc/abis_nm.h @@ -29,9 +29,6 @@ #include <osmocom/bsc/gsm_data.h> #include <osmocom/bsc/signal.h> -/* max number of attributes represented as 3GPP TS 52.021 §9.4.62 SW Description array */ -#define MAX_BTS_ATTR 5 - /* The BCCH info from an ip.access test, in host byte order * and already parsed... */ struct ipac_bcch_info { diff --git a/src/osmo-bsc/abis_nm.c b/src/osmo-bsc/abis_nm.c index 1b544fe..90b28eb 100644 --- a/src/osmo-bsc/abis_nm.c +++ b/src/osmo-bsc/abis_nm.c @@ -58,6 +58,9 @@ #define OM_HEADROOM_SIZE 128 #define IPACC_SEGMENT_SIZE 245 +/* max number of SW Description IEs we can parse */ +#define SW_DESCR_MAX 5 + #define LOGPMO(mo, ss, lvl, fmt, args ...) \ LOGP(ss, lvl, "OC=%s(%02x) INST=(%02x,%02x,%02x): " fmt, \ get_value_string(abis_nm_obj_class_names, (mo)->obj_class), \ @@ -610,7 +613,6 @@ uint16_t port; struct in_addr ia = {0}; char unit_id[40]; - struct abis_nm_sw_desc sw_descr[MAX_BTS_ATTR]; switch (bts->type) { case GSM_BTS_TYPE_OSMOBTS: @@ -638,6 +640,7 @@ /* Parse Attribute Response Info content for 3GPP TS 52.021 §9.4.61 SW Configuration */ if (TLVP_PRESENT(tp, NM_ATT_SW_CONFIG)) { + struct abis_nm_sw_desc sw_descr[SW_DESCR_MAX]; data = TLVP_VAL(tp, NM_ATT_SW_CONFIG); len = TLVP_LEN(tp, NM_ATT_SW_CONFIG); /* after parsing manufacturer-specific attributes there's list of replies in form of sw-conf structure: */ @@ -749,7 +752,7 @@ struct tlv_parsed tp; const uint8_t *sw_config; int ret, sw_config_len, len; - struct abis_nm_sw_desc sw_descr[MAX_BTS_ATTR]; + struct abis_nm_sw_desc sw_descr[SW_DESCR_MAX]; DEBUGPFOH(DNM, foh, "Software Activate Request, ACKing and Activating\n"); diff --git a/src/osmo-bsc/nm_bb_transc_fsm.c b/src/osmo-bsc/nm_bb_transc_fsm.c index 4b5e2b2..b68dc67 100644 --- a/src/osmo-bsc/nm_bb_transc_fsm.c +++ b/src/osmo-bsc/nm_bb_transc_fsm.c @@ -111,7 +111,7 @@ /* Request TRX-level attributes */ if (!bb_transc->mo.get_attr_sent && !bb_transc->mo.get_attr_rep_received) { - uint8_t attr_buf[MAX_BTS_ATTR]; + uint8_t attr_buf[3]; /* enlarge if needed */ uint8_t *ptr = &attr_buf[0]; *(ptr++) = NM_ATT_MANUF_STATE; diff --git a/src/osmo-bsc/nm_bts_fsm.c b/src/osmo-bsc/nm_bts_fsm.c index a9b6604..544efb4 100644 --- a/src/osmo-bsc/nm_bts_fsm.c +++ b/src/osmo-bsc/nm_bts_fsm.c @@ -98,7 +98,7 @@ /* Request generic BTS-level attributes */ if (!bts->mo.get_attr_sent && !bts->mo.get_attr_rep_received) { - uint8_t attr_buf[MAX_BTS_ATTR]; + uint8_t attr_buf[3]; /* enlarge if needed */ uint8_t *ptr = &attr_buf[0]; *(ptr++) = NM_ATT_MANUF_ID; -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/34355?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Idd8b971d32cf0f7a910a664d921e644b7c32d831 Gerrit-Change-Number: 34355 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 year, 9 months

1
0
0 0

[M] Change in osmo-mgw[master]: mgcp_client_fsm: fix inconsistent API (param_present, param).

by Jenkins Builder

Attention is currently required from: pespin. Jenkins Builder has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email ) Change subject: mgcp_client_fsm: fix inconsistent API (param_present, param). ...................................................................... Patch Set 3: (6 comments) File TODO-RELEASE: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10995): https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/6ead3e24_95605b7d PS3, Line 29: mgcp NEW API add struct struct mgcp_codec_param2, depreacte struct mgcp_codec_param 'depreacte' may be misspelled - perhaps 'deprecate'? File include/osmocom/mgcp_client/mgcp_client.h: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10995): https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/91762eba_b7461eca PS3, Line 139: bool param_present; /* Depreacted, please use codecs_param_present */ 'Depreacted' may be misspelled - perhaps 'Deprecated'? Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10995): https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/97f06dbb_ffa2ef8b PS3, Line 140: struct mgcp_codec_param param; /* Depreacted, please use codecs_param */ 'Depreacted' may be misspelled - perhaps 'Deprecated'? File src/libosmo-mgcp-client/mgcp_client.c: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10995): https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/7053e1fa_927718f1 PS3, Line 1376: LOGPMGW(mgcp, LOGL_NOTICE, "using depreacted struct member mgcp_msg->param, please use mgcp_msg->codecs_param!\n"); 'depreacted' may be misspelled - perhaps 'deprecated'? Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10995): https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/3f5e2528_67706bc8 PS3, Line 1393: if (mgcp_msg->codecs[i] != CODEC_AMR_8000_1 && mgcp_msg->codecs[i] != CODEC_AMRWB_16000_1) suspect code indent for conditional statements (24, 35) Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10995): https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/e2e47c09_b920d61c PS3, Line 1394: continue; Statements should start on a tabstop -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I50d737f3f3d45e4004c64101700a471fe75b3436 Gerrit-Change-Number: 34351 Gerrit-PatchSet: 3 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 13 Sep 2023 10:11:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 year, 9 months

1
0
0 0

[S] Change in osmo-mgw[master]: mgcp_client_fsm: explain member param in struct mgcp_conn_peer better

by dexter

Attention is currently required from: laforge, pespin. Hello Jenkins Builder, laforge, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/34350?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Code-Review+1 by laforge, Verified+1 by Jenkins Builder Change subject: mgcp_client_fsm: explain member param in struct mgcp_conn_peer better ...................................................................... mgcp_client_fsm: explain member param in struct mgcp_conn_peer better The struct member param specifies additional codec parameters. Let's improve its explaination. Change-Id: Iea4dc1e72fccaa464ce503fae88b5d8a867b1d19 Related: OS#6171 --- M include/osmocom/mgcp_client/mgcp_client_fsm.h 1 file changed, 15 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/50/34350/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34350?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Iea4dc1e72fccaa464ce503fae88b5d8a867b1d19 Gerrit-Change-Number: 34350 Gerrit-PatchSet: 3 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 year, 9 months

1
0
0 0

[S] Change in osmo-mgw[master]: mgcp_client_fsm: explain member param in struct mgcp_conn_peer better

by dexter

Attention is currently required from: laforge, pespin. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34350?usp=email ) Change subject: mgcp_client_fsm: explain member param in struct mgcp_conn_peer better ...................................................................... Patch Set 3: (1 comment) File include/osmocom/mgcp_client/mgcp_client_fsm.h: https://gerrit.osmocom.org/c/osmo-mgw/+/34350/comment/8ac9e092_74e5b1db PS2, Line 61: /*! Global codec params. In case the codec requires additional format parameters (fmtp), those cann be set > you could fix the "cann" typo while at it ;) Done -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34350?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Iea4dc1e72fccaa464ce503fae88b5d8a867b1d19 Gerrit-Change-Number: 34350 Gerrit-PatchSet: 3 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 13 Sep 2023 10:11:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 year, 9 months

1
0
0 0

[M] Change in osmo-mgw[master]: mgcp_client_fsm: fix inconsistent API (param_present, param).

by dexter

Attention is currently required from: pespin. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: mgcp_client_fsm: fix inconsistent API (param_present, param). ...................................................................... mgcp_client_fsm: fix inconsistent API (param_present, param). The struct struct mgcp_conn_peer in mgcp_client_fsm.h (and the struct struct mgcp_msg in mgcp_client.h) use a problematic way to hold information about codec parameters: Using the struct member param, the user can set options for AMR (octet-aligned/bandwith-efficient). This is done using the struct mgcp_codec_param, which is also not very well thought out. Since we can only set the parameters for AMR exactly once, it is not possible to set different parameters when AMR occurs multiple times in codecs. Since what we would have needed is an array of struct mgcp_codec_param, where we can set the parameters for each codec we use indvidually. So let's depreacate the use of param/param_present and add a new struct member codec_params/codec_params_present, where codec_params is an array. Let's also re-define mgcp_codec_params as mgcp_codec_params2, and use a union so that we can add params for other codecs than AMR in the future. Unfortunately mgcp_codec_params is also used in osmo-mgw, we should migrate each occurrence of mgcp_codec_param to mgcp_codec_param2 there too, but this should be done in a follow up patch. Related: OS#6171 Change-Id: I50d737f3f3d45e4004c64101700a471fe75b3436 --- M TODO-RELEASE M include/osmocom/mgcp/mgcp_common.h M include/osmocom/mgcp_client/mgcp_client.h M include/osmocom/mgcp_client/mgcp_client_fsm.h M src/libosmo-mgcp-client/mgcp_client.c M src/libosmo-mgcp-client/mgcp_client_fsm.c 6 files changed, 84 insertions(+), 7 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/51/34351/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I50d737f3f3d45e4004c64101700a471fe75b3436 Gerrit-Change-Number: 34351 Gerrit-PatchSet: 3 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 year, 9 months

1
0
0 0

[M] Change in osmo-mgw[master]: mgcp_client_fsm: fix inconsistent API (param_present, param).

by dexter

Attention is currently required from: pespin. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email ) Change subject: mgcp_client_fsm: fix inconsistent API (param_present, param). ...................................................................... Patch Set 3: (3 comments) File include/osmocom/mgcp_client/mgcp_client.h: https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/06773548_17c9da3c PS2, Line 121: struct mgcp_msg { > If this struct is being allocated outside of the library, now it's for sure the moment to add an all […] I have checked this one. It is not used from outside. I think there would also be no point in doing so since this struct models the MGCP messages that are exchanged and the purpose of the mgcp-client is to keep exactly that away from the user. File include/osmocom/mgcp_client/mgcp_client_fsm.h: https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/4de29969_8ba2d58c PS2, Line 70: struct mgcp_codec_param2 codecs_param[MGCP_MAX_CODECS]; > as soon as you add a field to "struct mgcp_codec_param2", the size of the struct will and change any […] Yes, that is a real problem ;-/ I will add an alloc function in a different patch. File src/libosmo-mgcp-client/mgcp_client.c: https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/bf4bce83_d5bb3ccb PS2, Line 1376: LOGPMGW(mgcp, LOGL_DEBUG, "using depreacted struct member mgcp_msg->param, please use mgcp_msg->codecs_param!\n"); > only debug? Done -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I50d737f3f3d45e4004c64101700a471fe75b3436 Gerrit-Change-Number: 34351 Gerrit-PatchSet: 3 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 13 Sep 2023 10:11:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 year, 9 months

1
0
0 0

[M] Change in osmo-mgw[master]: mgcp_client_fsm: allow the same codec multiple times in ptmap

by dexter

Attention is currently required from: dexter, pespin. Hello Jenkins Builder, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/34349?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: mgcp_client_fsm: allow the same codec multiple times in ptmap ...................................................................... mgcp_client_fsm: allow the same codec multiple times in ptmap In struct mgcp_conn_peer, the API user is supposed to fill in the codecs he wants to use and optionally a payload type map (ptmap). The ptmap is used to assign a user defined payload type number for each codec. (in case ptmap is not used, the IANA/3GPP assigned payload type numbers apply) Unfortunately ptmap works like a simple lookup table. When a payload type is looked up for a specific codec, then the lookup table is searched for the codec and the payload type of the first entry that is found is returned. This works fine as long as the same codec is not apparing twice, which different payload type numbers. What seems to be a corner case on the first look turns out to be a common case with AMR, where a bandwith-efficient mode and an octet-aligned mode may be specified. Then we need to specify AMR twice, but with different payload type numbers in ptmap. So, let's equip the function that does the mapping (map_pt_to_codec) with a memory, so that it can checkmark entries it has already mapped. This will allow us to put the same codec type multiple times in ptmap and as long as the order in ptmap matches the order used in codecs the mapping will be consistent. Related: OS#6171 Change-Id: Ie13ce59d3165936a16e16588b4d58f0ce7e0ae67 --- M include/osmocom/mgcp_client/mgcp_client.h M include/osmocom/mgcp_client/mgcp_client_fsm.h M src/libosmo-mgcp-client/mgcp_client.c M tests/mgcp_client/mgcp_client_test.c M tests/mgcp_client/mgcp_client_test.ok 5 files changed, 112 insertions(+), 8 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/49/34349/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34349?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ie13ce59d3165936a16e16588b4d58f0ce7e0ae67 Gerrit-Change-Number: 34349 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 year, 9 months

1
0
0 0

[M] Change in osmo-mgw[master]: mgcp_client_fsm: allow the same codec multiple times in ptmap

by Jenkins Builder

Attention is currently required from: pespin. Jenkins Builder has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34349?usp=email ) Change subject: mgcp_client_fsm: allow the same codec multiple times in ptmap ...................................................................... Patch Set 3: (1 comment) File src/libosmo-mgcp-client/mgcp_client.c: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10992): https://gerrit.osmocom.org/c/osmo-mgw/+/34349/comment/e96d8236_33aaa8e1 PS3, Line 179: skip: labels should not be indented -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34349?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ie13ce59d3165936a16e16588b4d58f0ce7e0ae67 Gerrit-Change-Number: 34349 Gerrit-PatchSet: 3 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 13 Sep 2023 09:28:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 year, 9 months

1
0
0 0

2025

2024

2023

2022

gerrit-log September 2023