Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_packet.c
line 72
@@ -334,6 +334,54 @@ void *_odp_packet_map(void *pkt_ptr, uint32_t offset,
uint32_t *seg_len,
return packet_map(pkt_ptr, offset, seg_len, seg_idx);
}
+int _odp_packet_copy_from_mem_seg(odp_packet_t pkt, uint32_t offset,
+ uint32_t len, const void *src)
+{
+ void *mapaddr;
+ uint32_t seglen = 0; /* GCC */
+ uint32_t cpylen;
+ const uint8_t *srcaddr = (const uint8_t *)src;
+ odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);
+
+ if (offset + len > pkt_hdr->frame_len)
+ return -1;
+
+ while (len > 0) {
+ mapaddr = packet_map(pkt_hdr, offset, &seglen, NULL);
+ cpylen = len > seglen ? seglen : len;
+ memcpy(mapaddr, srcaddr, cpylen);
+ offset += cpylen;
+ srcaddr += cpylen;
+ len -= cpylen;
+ }
+
+ return 0;
+}
+
+int _odp_packet_copy_to_mem_seg(odp_packet_t pkt, uint32_t offset,
+ uint32_t len, void *dst)
+{
+ void *mapaddr;
+ uint32_t seglen = 0; /* GCC */
+ uint32_t cpylen;
+ uint8_t *dstaddr = (uint8_t *)dst;
+ odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);
+
+ if (offset + len > pkt_hdr->frame_len)
+ return -1;
+
+ while (len > 0) {
+ mapaddr = packet_map(pkt_hdr, offset, &seglen, NULL);
+ cpylen = len > seglen ? seglen : len;
+ memcpy(dstaddr, mapaddr, cpylen);
+ offset += cpylen;
+ dstaddr += cpylen;
+ len -= cpylen;
+ }
+
+ return 0;
+}
Comment:
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_r165277448
updated_at 2018-02-01 07:36:12
