Attention is currently required from: neels.

View Change

4 comments:

• File include/osmocom/tlv/tlv.h:

  • Patch Set #1, Line 102: unsigned int tag;

    Isn't this duplicated in load_tl callback?

• File src/libosmo-tlv/tlv.c:

  • Patch Set #1, 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.

  • Patch Set #1, 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.

  • Patch Set #1, 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 change 27217. To unsubscribe, or for help writing mail filters, visit settings.