Dmitry Eremin-Solenikov(lumag) replied on github web page: platform/linux-generic/odp_ipsec.c line 245 @@ -1165,6 +1167,8 @@ static int ipsec_out_esp(odp_packet_t *pkt, ipsec_offset + _ODP_ESPHDR_LEN, ipsec_sa->esp_iv_len, state->iv + ipsec_sa->salt_length); + _odp_packet_set_data(*pkt, esptrl_offset - esptrl.pad_len - tfc_len, + 0xa5, tfc_len);
Comment: Well, it's just a magic number. Same as 0 would be. > Dmitry Eremin-Solenikov(lumag) wrote: > Hmm. We've spent several minutes on this, but nobody reminded of API > convention. Should we change the spec here? @psavol what is your opinion? >> Dmitry Eremin-Solenikov(lumag) wrote: >> We require that an application sets `l3_offset` for all packet it pushes to >> IPsec. For TFC dummy packets it resulted in `l3_offset` being set but >> ignored. Thus I proposed this change. Other solution might be to stop >> requiring `l3_offset` for TFC dummy packets. >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> Should `0xa5` be a `#define` rather than a "magic number"? >>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>> Nit: could use `odp_unlikely()` here. >>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>> This change requires an API change as the spec says relevant offsets must >>>>> be in the range `0..odp_packet_len(pkt) - 1` . Same comment for the L3 >>>>> and L4 changes in this patch. >>>>> >>>>> In theory the validation tests should test these bounds, but as with most >>>>> parts of the API violations simply result in undefined behavior, so this >>>>> is an "honor system". Still, we can't violate the spec here without >>>>> changing the spec. >>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>> Unless it has been parsed, `odp_packet_l3_offset()` is initialized to >>>>>> `ODP_PACKET_OFFSET_INVALID`, so this seems an undue burden. The original >>>>>> wording seems cleaner from an application perspective. https://github.com/Linaro/odp/pull/494#discussion_r169886818 updated_at 2018-02-22 08:47:03