Attention is currently required from: neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-upf/+/27217 ) Change subject: libosmo-tlv: add versatile TLV de- and encoder ...................................................................... Patch Set 1: (4 comments) File include/osmocom/tlv/tlv.h: https://gerrit.osmocom.org/c/osmo-upf/+/27217/comment/d52ae8f0_aa2de0ac PS1, Line 102: unsigned int tag; Isn't this duplicated in load_tl callback? File src/libosmo-tlv/tlv.c: https://gerrit.osmocom.org/c/osmo-upf/+/27217/comment/c5393a85_847a0f4b PS1, Line 256: static int t16l16v_load_tl(unsigned int *tag, size_t *len, const uint8_t **val, See my comment below for store_tl, it also applies here. https://gerrit.osmocom.org/c/osmo-upf/+/27217/comment/ce0d5b13_c0dcb6bf PS1, Line 266: static int t16l16v_store_tl(uint8_t *dst_data, size_t dst_data_avail, unsigned int tag, size_t len, From my experience after looking at open5gs TLV parser, these callbacks can avoided. As you can see, t16l16v_store_tl and t8l8v_store_tl are basically doing the same steps, but with different parameters. So you can avoid users needs to implement functions or this code calling functions pointers by simply storing some parameters in osmo_tlv_cfg: uint8_t tag_byte_len; uint8_t length_byte_len; You could also add tag_max_val = UINT16_MAX to the struct, and so on. Then this store_tl function can use all that anb become a static generic function. https://gerrit.osmocom.org/c/osmo-upf/+/27217/comment/a0a4f786_bd224b55 PS1, Line 275: osmo_store16be(tag, dst_data); You may want to also support setting instance in the API, to avoid API breakage in the future when we want to use it in more places. See open5gs lib/core/*tlv*.{c,h} for different types of TLVs. https://github.com/sysmocom/open5gs/blob/pespin/gtp1/lib/core/ogs-tlv.h#L32 The presence of Instance byte can also be then configured in tlv_config->instance_byte_len. -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/27217 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: Ib0fd00d9f288ffe13b7e67701f3e47073587404a Gerrit-Change-Number: 27217 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Thu, 17 Feb 2022 13:06:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment