Dmitry Eremin-Solenikov(lumag) replied on github web page:
test/validation/api/pktio/pktio.c
line 16
@@ -1195,10 +1195,8 @@ void pktio_test_index(void)
ndx = odp_pktio_index(pktio);
CU_ASSERT(ndx >= 0);
- CU_ASSERT(odp_pktio_index(pktio_inval) < 0);
CU_ASSERT(odp_pktio_close(pktio) == 0);
- CU_ASSERT(odp_pktio_index(pktio) < 0);
Comment:
@psavol Totally agree about close + index.
> Petri Savolainen(psavol) wrote:
> To me "(e.g. handle not valid)" does not yet guarantee the check. This would:
> "<0 When pktio handle is invalid, or some other error happened".
>
> In general, we should remove all such "e.g. handle not valid" comments.
>> Petri Savolainen(psavol) wrote:
>> API does not (and should not) guarantee that invalid or stale handles are
>> checked on fast path functions, such as this conversion to index.
>> Especially, any API function is not specified to check against stale
>> handles. Function may return -1, if it finds an error but we don't guarantee
>> which errors are checked. For example, there may be check against invalid
>> handle, but there may not be either. Invalid values are mostly needed for
>> API function to be able return that e.g. create/open/alloc failed. E.g.
>> here, application should check that pktio open succeeded, if it did not
>> succeed it should not continue and pass that invalid handle to other pktio
>> calls. That way every API call does not need to check the handle, only
>> application needs to check it once after open/create/alloc.
>>
>> This close + index test above it basically the same thing as doing:
>> odp_packet_free(pkt);
>> data = odp_packet_data(pkt);
>> // now we can assume that data is NULL, since pkt packet was freed and this
>> function always searches through valid handles and notices that this handle
>> value is not valid anymore.
>>> Petri Savolainen(psavol) wrote:
>>> Those need to be inside #include <odp/visibility_begin.h> #include
>>> <odp/visibility_end.h>. It's cleaner to have one vs multiple such regions
>>> in one c file.
>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>> We can have `_odp_pktio_index()` which does not perform such checks and
>>>> `odp_pktio_index()` which does `ODP_PKTIO_INVALID` check.
>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>> Generally I would agree. However we explicitly have the invalid usecase
>>>>> in API:
>>>>> ```
>>>>> @retval <0 On failure (e.g., handle not valid)
>>>>> ```
>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>> Is there a reason to move function bodies?
>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>> Maybe `(uintptr_t)(handle) == (uintptr_t)0` would do the trick?
>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>> Very strange. I'm Ok with this workaround, but a comment about clang
>>>>>>>> version might be appropriate. Don't know if it's worth doing a clang
>>>>>>>> version check. We do things like that in the `ODP_STATIC_ASSERT()`
>>>>>>>> macro itself for similar reasons.
>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>> odp_packet.c:52:19: error: static_assert expression is not an
>>>>>>>>> integral constant
>>>>>>>>> expression
>>>>>>>>> ODP_STATIC_ASSERT(_odp_typeval(ODP_BUFFER_INVALID) == 0, "Buffer
>>>>>>>>> inval not 0");
>>>>>>>>> ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>> ../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:30:
>>>>>>>>> note:
>>>>>>>>> expanded from macro '_odp_typeval'
>>>>>>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>>>>>> ^
>>>>>>>>> ../../include/odp/api/abi-default/debug.h:26:53: note: expanded from
>>>>>>>>> macro
>>>>>>>>> 'ODP_STATIC_ASSERT'
>>>>>>>>> #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)
>>>>>>>>> ^~~~
>>>>>>>>> odp_packet.c:52:19: note: cast that performs the conversions of a
>>>>>>>>> reinterpret_cast is not allowed in a constant expression
>>>>>>>>> ../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:41:
>>>>>>>>> note:
>>>>>>>>> expanded from macro '_odp_typeval'
>>>>>>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>> static_assert() seems special. The clang version in Travis does not
>>>>>>>>>> accept type casts with static_assert(). The cast confuses it to
>>>>>>>>>> think that (uintptr_t)0 == 0 is not an integral constant.
>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>> `_odp_typeval()` is defined as:
>>>>>>>>>>> ```
>>>>>>>>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>>>>>>>> ```
>>>>>>>>>>> It's used elsewhere in the ODP code and clang doesn't have any
>>>>>>>>>>> problem with it. I've verified this works for clang 4.2.1. Since
>>>>>>>>>>> it works elsewhere on Travis I'm not sure why it wouldn't in this
>>>>>>>>>>> instance.
>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>> I'm OK with just a modified commit name. I agree we needn't be too
>>>>>>>>>>>> granular about these things.
>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>> I could rename the patch to:
>>>>>>>>>>>>> "linux-gen: packet: use inlined flow hash and ts set"
>>>>>>>>>>>>>
>>>>>>>>>>>>> ... if necessary. The comment says already that also ts set is
>>>>>>>>>>>>> modified the same way.
>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>> I did try several combinations of type casts of invalid values,
>>>>>>>>>>>>>> also cast to uintptr_t. The problem is that gcc and clang (clang
>>>>>>>>>>>>>> version 3.8.0-2ubuntu4) of my machine accepted all of those
>>>>>>>>>>>>>> casts, but the clang version in Travis (clang version
>>>>>>>>>>>>>> 3.8.0-2ubuntu3~trusty5) does not accept anything.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Maybe it's a bug in the particular version of clang. Anyway,
>>>>>>>>>>>>>> addition of this check even only on gcc side is improvement
>>>>>>>>>>>>>> against what we currently have (no check). Packet invalid is
>>>>>>>>>>>>>> currently 0xfffffff in ABI spec, but event/buffer invalid are 0.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> odp_packet.c:55:19: error: static_assert expression is not an
>>>>>>>>>>>>>> integral constant
>>>>>>>>>>>>>> expression
>>>>>>>>>>>>>> ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet
>>>>>>>>>>>>>> invalid not 0");
>>>>>>>>>>>>>> ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>>>>>>> ../../include/odp/api/abi-default/debug.h:26:53: note: expanded
>>>>>>>>>>>>>> from macro
>>>>>>>>>>>>>> 'ODP_STATIC_ASSERT'
>>>>>>>>>>>>>> #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)
>>>>>>>>>>>>>> ^~~~
>>>>>>>>>>>>>> odp_packet.c:55:20: note: cast that performs the conversions of a
>>>>>>>>>>>>>> reinterpret_cast is not allowed in a constant expression
>>>>>>>>>>>>>> ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet
>>>>>>>>>>>>>> invalid not 0");
>>>>>>>>>>>>>> ^
>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>> Since you want to breech the strong type you need to use the
>>>>>>>>>>>>>>> internal `odp_typeval()` macro here.
>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>> ODP_STATIC_ASSERT(_odp_typeval(ODP_EVENT_INVALID) == 0, "Event
>>>>>>>>>>>>>>> invalid not 0");
>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>> etc. I've verified that this works fine for clang, so no need
>>>>>>>>>>>>>>> for the conditional compilation.
>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>> Should this be in its own commit? Different function.
https://github.com/Linaro/odp/pull/437#discussion_r165285392
updated_at 2018-02-01 08:29:14
