<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22046">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22046/2/include/osmocom/gprs/gprs_bssgp.h">File include/osmocom/gprs/gprs_bssgp.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/22046/2/include/osmocom/gprs/gprs_bssgp.h@45">Patch Set #2, Line 45:</a> <code style="font-family:monospace,monospace">struct bssgp_ran_information_pdu {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not really liking the current approach with one struct having pointers to already encoded IEs, it's kind of cumbersome because you have to iteratively encode stuff and store it in temporary buffers in code using it. See for instance the draft I wrote for osmo-pcu:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">"""<br>static void fill_rim_ri(struct bssgp_rim_routing_info *info, const struct osmo_cell_global_id_ps *cgi_ps)<br>{<br> *info = {<br> .discr = BSSGP_RIM_ROUTING_INFO_GERAN,<br> .geran = {<br> .raid = {<br> .mcc = cgi_ps->rai.lac.plmn.mcc,<br> .mnc = cgi_ps->rai.lac.plmn.mnc,<br> .mnc_3_digits = cgi_ps->rai.lac.plmn.mnc_3_digits,<br> .lac = cgi_ps->rai.lac.lac,<br> .rac = cgi_ps->rai.rac,<br> },<br> .cid = cgi_ps->cell_identity,<br> },<br> };<br>}</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static struct ran_information_pdu *gen_rim_ran_info_req(struct nacc_fsm_ctx *ctx)<br>{<br> struct ran_information_pdu *req = talloc_zero(ctx, struct ran_information_pdu);<br> struct gprs_rlcmac_bts *bts = bts_data(ctx->ms->bts);<br> struct bssgp_rim_routing_info src, dst;<br> uint8_t *src_buf = talloc_size(ctx, 64);<br> uint8_t *dst_buf = talloc_size(ctx, 64);<br> uint8_t *cont_buf = talloc_size(ctx, 64);<br> int rc, src_len, dst_len, cont_len;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> fill_rim_ri(&dst, ctx->cgi_ps);<br> dst_len = bssgp_create_rim_ri(dst_buf, &dst);<br> if (dst_len <= 0)<br> goto err_ret;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> memset(&src, 0, sizeof(src)); //TODO: fill correctly from BTS data<br> src->discr = BSSGP_RIM_ROUTING_INFO_GERAN;<br> src_len = bssgp_create_rim_ri(src_buf, &src);<br> if (src_len <= 0)<br> goto err_ret;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct bssgp_ran_inf_req_rim_cont cont = {<br> .app_id = BSSGP_RAN_INF_APP_ID_NACC;<br> .seq_num = 1;<br> .pdu_ind.ack_requested = 0;<br> .pdu_ind.pdu_type_ext = 1;<br> .prot_ver = 1;<br> .son_trans_app_id = NULL;<br> .son_trans_app_id_len = 0;<br> .u = {<br> .app_cont_nacc = {<br> .reprt_cell = {<br> .raid = {<br> .mcc = ctx->cgi_ps.rai.lac.plmn.mcc,<br> .mnc = ctx->cgi_ps.rai.lac.plmn.mnc,<br> .mnc_3_digits = false, //FIXME, how to say true/false here?<br> .lac = ctx->cgi_ps.rai.lac.lac,<br> .rac= ctx->cgi_ps.rai.rac,<br> },<br> .cid = bts->cell_id,<br> },<br> },<br> },<br> };<br> cont_len = bssgp_enc_ran_inf_req_rim_cont(cont_buf, 64, &cont);<br> if (cont_len <= 0)<br> goto err_ret;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct ran_information_pdu rim = {<br> .bssgp_pdu_type = BSSGP_PDUT_RAN_INFO_REQ,<br> .d_cid = dst_buf,<br> .d_cid_len = dst_len,<br> .s_cid = src_buf,<br> .s_cid_len = src_len,<br> .rim_cont = cont,<br> .rim_cont_len = cont_len,<br> .rim_cont_iei = 0x57,<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> return req;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">err_ret:<br> talloc_free(req);<br> return NULL;<br>}<br>"""</pre><p style="white-space: pre-wrap; word-wrap: break-word;"><br>So why not having the relevant decoded structs (such as struct bssgp_rim_routing_info) into the struct and do the encoding in 1 pass insterad of having several passes?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Same as per decoding. I would expect the decode function to already provide me with 1 full struct with all protocol related parts decoded.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/22046">change 22046</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/22046"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I18134fd9938040d2facb6beee3732628b167ce8c </div>
<div style="display:none"> Gerrit-Change-Number: 22046 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 08 Jan 2021 17:06:22 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>