| Message ID | 1418904708-1412-4-git-send-email-taras.kondratiuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
See detailed comments inline. I verified that this passes all tests when the packet segmentation patch is applied. Bill On Thu, Dec 18, 2014 at 6:11 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > > Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > --- > test/validation/Makefile.am | 1 + > test/validation/buffer/odp_buffer_testsuites.h | 4 + > test/validation/buffer/odp_packet_test.c | 670 > +++++++++++++++++++++++++ > test/validation/odp_buffer.c | 5 + > 4 files changed, 680 insertions(+) > create mode 100644 test/validation/buffer/odp_packet_test.c > > diff --git a/test/validation/Makefile.am b/test/validation/Makefile.am > index bbec23f..48c7fbd 100644 > --- a/test/validation/Makefile.am > +++ b/test/validation/Makefile.am > @@ -21,6 +21,7 @@ dist_odp_shm_SOURCES = odp_shm.c > common/odp_cunit_common.c > dist_odp_schedule_SOURCES = odp_schedule.c common/odp_cunit_common.c > dist_odp_buffer_SOURCES = buffer/odp_buffer_pool_test.c \ > buffer/odp_buffer_test.c \ > + buffer/odp_packet_test.c \ > odp_buffer.c common/odp_cunit_common.c > > #For Linux generic the unimplemented crypto API functions break the > diff --git a/test/validation/buffer/odp_buffer_testsuites.h > b/test/validation/buffer/odp_buffer_testsuites.h > index ca42c2d..715d9ac 100644 > --- a/test/validation/buffer/odp_buffer_testsuites.h > +++ b/test/validation/buffer/odp_buffer_testsuites.h > @@ -16,10 +16,14 @@ > > extern CU_TestInfo buffer_pool_tests[]; > extern CU_TestInfo buffer_tests[]; > +extern CU_TestInfo packet_tests[]; > > extern int buffer_testsuite_init(void); > extern int buffer_testsuite_finalize(void); > > +extern int packet_testsuite_init(void); > +extern int packet_testsuite_finalize(void); > + > odp_buffer_pool_t pool_create(int buf_num, int buf_size, int buf_type); > > #endif /* ODP_BUFFER_TESTSUITES_H_ */ > diff --git a/test/validation/buffer/odp_packet_test.c > b/test/validation/buffer/odp_packet_test.c > new file mode 100644 > index 0000000..a664fe5 > --- /dev/null > +++ b/test/validation/buffer/odp_packet_test.c > @@ -0,0 +1,670 @@ > +/* Copyright (c) 2014, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +#include "odp_buffer_testsuites.h" > +#include <stdlib.h> > + > +#define PACKET_BUF_LEN ODP_CONFIG_PACKET_BUF_LEN_MIN > +/* Reserve some tailroom for tests */ > +#define PACKET_TAILROOM_RESERVE 4 > + > +static odp_buffer_pool_t packet_pool; > +static const uint32_t packet_len = PACKET_BUF_LEN - > + ODP_CONFIG_PACKET_HEADROOM - > + ODP_CONFIG_PACKET_TAILROOM - > + PACKET_TAILROOM_RESERVE; > + > +odp_packet_t test_packet; > + > +int packet_testsuite_init(void) > +{ > + odp_buffer_pool_param_t params = { > + .buf_size = PACKET_BUF_LEN, > + .buf_align = ODP_CACHE_LINE_SIZE, > + .num_bufs = 100, > + .buf_type = ODP_BUFFER_TYPE_PACKET, > + }; > + > + packet_pool = odp_buffer_pool_create("packet_pool", > ODP_SHM_INVALID, > + ¶ms); > + if (packet_pool == ODP_BUFFER_POOL_INVALID) > + return -1; > + > + test_packet = odp_packet_alloc(packet_pool, packet_len); > + if (odp_packet_is_valid(test_packet) == 0) > + return -1; > + > + return 0; > +} > + > +int packet_testsuite_finalize(void) > +{ > + odp_packet_free(test_packet); > + if (odp_buffer_pool_destroy(packet_pool) != 0) > + return -1; > + return 0; > +} > + > +static void packet_alloc_free(void) > +{ > + odp_buffer_pool_t pool; > + odp_packet_t packet; > + pool = pool_create(1, PACKET_BUF_LEN, ODP_BUFFER_TYPE_PACKET); > + > + /* Allocate the only buffer from the pool */ > + packet = odp_packet_alloc(pool, packet_len); > + CU_ASSERT_FATAL(packet != ODP_PACKET_INVALID); > + CU_ASSERT(odp_packet_len(packet) == packet_len); > + /** @todo: is it correct to assume the pool had only one buffer? */ > + CU_ASSERT_FATAL(odp_packet_alloc(pool, packet_len) == > ODP_PACKET_INVALID) > + > + odp_packet_free(packet); > + > + /* Check that the buffer was returned back to the pool */ > + packet = odp_packet_alloc(pool, packet_len); > + CU_ASSERT_FATAL(packet != ODP_PACKET_INVALID); > + CU_ASSERT(odp_packet_len(packet) == packet_len); > + > + odp_packet_free(packet); > + CU_ASSERT(odp_buffer_pool_destroy(pool) == 0); > +} > + > +static void packet_alloc_segmented(void) > +{ > + odp_packet_t pkt; > + const uint32_t len = ODP_CONFIG_PACKET_BUF_LEN_MAX - > + ODP_CONFIG_PACKET_HEADROOM - > + ODP_CONFIG_PACKET_TAILROOM; > + > + pkt = odp_packet_alloc(packet_pool, len); > + CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID); > + CU_ASSERT(odp_packet_len(pkt) == len); > + odp_packet_free(pkt); > +} > + > +static void packet_buffer_conversion(void) > +{ > + odp_packet_t pkt = test_packet; > + odp_packet_t tmp_pkt; > + odp_buffer_t buf; > + > + buf = odp_packet_to_buffer(pkt); > + CU_ASSERT_FATAL(buf != ODP_BUFFER_INVALID); > + CU_ASSERT(odp_buffer_type(buf) == ODP_BUFFER_TYPE_PACKET); > + CU_ASSERT(odp_buffer_size(buf) == odp_packet_buf_len(pkt)); > + > + tmp_pkt = odp_packet_from_buffer(buf); > + CU_ASSERT_FATAL(tmp_pkt != ODP_PACKET_INVALID); > + /** @todo: Need an API to compare packets */ > +} > + > +static void packet_basic_metadata(void) > +{ > + odp_packet_t pkt = test_packet; > + CU_ASSERT(odp_packet_head(pkt) != NULL); > + CU_ASSERT(odp_packet_data(pkt) != NULL); > + > + CU_ASSERT(odp_packet_pool(pkt) != ODP_BUFFER_POOL_INVALID); > + /* Packet was allocated by application so shouldn't have valid > pktio. */ > + CU_ASSERT(odp_packet_input(pkt) == ODP_PKTIO_INVALID); > +} > + > +static void packet_length(void) > +{ > + odp_packet_t pkt = test_packet; > + uint32_t buf_len, headroom, tailroom; > + > + buf_len = odp_packet_buf_len(pkt); > + headroom = odp_packet_headroom(pkt); > + tailroom = odp_packet_tailroom(pkt); > + > + CU_ASSERT(odp_packet_len(pkt) == packet_len); > + CU_ASSERT(headroom >= ODP_CONFIG_PACKET_HEADROOM); > As we discussed, should have a guard against ODP_CONFIG_PACKET_HEADROOM being 0 similar to what you're doing with ODP_CONFIG_PACKET_TAILROOM since it's legit to specify that config var as 0. > +#if ODP_CONFIG_PACKET_TAILROOM != 0 /* Avoid 'always true' warning */ > + CU_ASSERT(tailroom >= ODP_CONFIG_PACKET_TAILROOM); > +#endif > + CU_ASSERT(buf_len >= packet_len + headroom + tailroom); > +} > + > +static void packet_debug(void) > +{ > + CU_ASSERT(odp_packet_is_valid(test_packet) == 1); > + odp_packet_print(test_packet); > +} > + > +static void packet_context(void) > +{ > + odp_packet_t pkt = test_packet; > + char ptr_test_value = 2; > + uint64_t u64_test_value = 0x0123456789abcdf; > + > + void *prev_ptr; > + uint64_t prev_u64; > + > + prev_ptr = odp_packet_user_ptr(pkt); > + odp_packet_user_ptr_set(pkt, &ptr_test_value); > + CU_ASSERT(odp_packet_user_ptr(pkt) == &ptr_test_value); > + odp_packet_user_ptr_set(pkt, prev_ptr); > + > + prev_u64 = odp_packet_user_u64(pkt); > + odp_packet_user_u64_set(pkt, u64_test_value); > + CU_ASSERT(odp_packet_user_u64(pkt) == u64_test_value); > + odp_packet_user_u64_set(pkt, prev_u64); > + > + odp_packet_reset(pkt, packet_len); > +} > + > +static void packet_layer_offsets(void) > +{ > + odp_packet_t pkt = test_packet; > + uint8_t *l2_addr, *l3_addr, *l4_addr; > + uint32_t seg_len; > + const uint32_t l2_off = 2; > + const uint32_t l3_off = l2_off + 14; > + const uint32_t l4_off = l3_off + 14; > + > + /* Set offsets to the same value */ > + odp_packet_l2_offset_set(pkt, l2_off); > + odp_packet_l3_offset_set(pkt, l2_off); > + odp_packet_l4_offset_set(pkt, l2_off); > + > + /* Addresses should be the same */ > + l2_addr = odp_packet_l2_ptr(pkt, &seg_len); > + CU_ASSERT(seg_len != 0); > + l3_addr = odp_packet_l3_ptr(pkt, &seg_len); > + CU_ASSERT(seg_len != 0); > + l4_addr = odp_packet_l4_ptr(pkt, &seg_len); > + CU_ASSERT(seg_len != 0); > + CU_ASSERT(l2_addr != NULL); > + CU_ASSERT(l2_addr == l3_addr); > + CU_ASSERT(l2_addr == l4_addr); > + > + /* Set offsets to the different values */ > + odp_packet_l2_offset_set(pkt, l2_off); > + CU_ASSERT(odp_packet_l2_offset(pkt) == l2_off); > + odp_packet_l3_offset_set(pkt, l3_off); > + CU_ASSERT(odp_packet_l3_offset(pkt) == l3_off); > + odp_packet_l4_offset_set(pkt, l4_off); > + CU_ASSERT(odp_packet_l4_offset(pkt) == l4_off); > + > + /* Addresses should not be the same */ > + l2_addr = odp_packet_l2_ptr(pkt, NULL); > + CU_ASSERT(l2_addr != NULL); > + l3_addr = odp_packet_l3_ptr(pkt, NULL); > + CU_ASSERT(l3_addr != NULL); > + l4_addr = odp_packet_l4_ptr(pkt, NULL); > + CU_ASSERT(l4_addr != NULL); > + > + CU_ASSERT(l2_addr != l3_addr); > + CU_ASSERT(l2_addr != l4_addr); > + CU_ASSERT(l3_addr != l4_addr); +} > + > +static void _verify_headroom_shift(odp_packet_t packet, > + int shift) > +{ > + uint32_t room = odp_packet_headroom(packet); > + uint32_t seg_data_len = odp_packet_seg_len(packet); > + uint32_t pkt_data_len = odp_packet_len(packet); > + void *data; > + char *data_orig = odp_packet_data(packet); > + char *head_orig = odp_packet_head(packet); > + > + if (shift == 0) > + return; > Actually, you should test that 0 does not change things, since a push/pull of 0 is legitimate. Delete this check, and change test below to if (shift >= 0). > + > + if (shift > 0) > + data = odp_packet_push_head(packet, shift); > + else > + data = odp_packet_pull_head(packet, -shift); > + > + CU_ASSERT(data != NULL); > Not always true. If shift is out of range NULL is the expected return. > + CU_ASSERT(odp_packet_headroom(packet) == room - shift); > + CU_ASSERT(odp_packet_seg_len(packet) == seg_data_len + shift); > + CU_ASSERT(odp_packet_len(packet) == pkt_data_len + shift); > + CU_ASSERT(odp_packet_data(packet) == data); + CU_ASSERT(odp_packet_head(packet) == head_orig); > + CU_ASSERT(data == data_orig - shift); > Given the revised definition of push/pull, this test should also verify that these operations do not change the various l2/l3/l4 offset values. > +} > + > +static void packet_headroom(void) > +{ > + odp_packet_t pkt = test_packet; > + uint32_t room; > + uint32_t seg_data_len; > + uint32_t push_val, pull_val; > + > + room = odp_packet_headroom(pkt); > + CU_ASSERT(room >= ODP_CONFIG_PACKET_HEADROOM); > + seg_data_len = odp_packet_seg_len(pkt); > + CU_ASSERT(seg_data_len >= 1); > + /** @todo: should be len - 1 */ > + pull_val = seg_data_len / 2; > + push_val = room; > + > + _verify_headroom_shift(pkt, -pull_val); > + _verify_headroom_shift(pkt, push_val + pull_val); > + _verify_headroom_shift(pkt, -push_val); > +} > + > +static void _verify_tailroom_shift(odp_packet_t pkt, > + int shift) > +{ > + odp_packet_seg_t seg; > + uint32_t room; > + uint32_t seg_data_len, pkt_data_len; > + void *tail; > + char *tail_orig; > + > + if (shift == 0) > + return; > Same as for headroom, it's legit to push/pull tail by zero, so this should be included to verify that 0 doesn't change anything. > + > + room = odp_packet_tailroom(pkt); > + pkt_data_len = odp_packet_len(pkt); > + tail_orig = odp_packet_tail(pkt); > + > + seg = odp_packet_last_seg(pkt); > + CU_ASSERT(seg != ODP_SEGMENT_INVALID); > + seg_data_len = odp_packet_seg_data_len(pkt, seg); > + > + if (shift > 0) > + tail = odp_packet_push_tail(pkt, shift); > + else > + tail = odp_packet_pull_tail(pkt, -shift); > + > + CU_ASSERT(tail != NULL); > Not always true. If shift is out of range NULL is the expected return. > + CU_ASSERT(odp_packet_seg_data_len(pkt, seg) == seg_data_len + > shift); > + CU_ASSERT(odp_packet_len(pkt) == pkt_data_len + shift); > + CU_ASSERT(odp_packet_tailroom(pkt) == room - shift); > + if (room == 0 || (room - shift) == 0) > + return; > + if (shift > 0) { > + CU_ASSERT(odp_packet_tail(pkt) == tail_orig + shift); > + CU_ASSERT(tail == tail_orig); > + } else { > + CU_ASSERT(odp_packet_tail(pkt) == tail); > + CU_ASSERT(tail == tail_orig + shift); > + } > +} > + > +static void packet_tailroom(void) > +{ > + odp_packet_t pkt = test_packet; > + odp_packet_seg_t segment; > + uint32_t room; > + uint32_t seg_data_len; > + uint32_t push_val, pull_val; > + > + segment = odp_packet_last_seg(pkt); > + CU_ASSERT(segment != ODP_SEGMENT_INVALID); > + room = odp_packet_tailroom(pkt); > +#if ODP_CONFIG_PACKET_TAILROOM != 0 /* Avoid 'always true' warning */ > + CU_ASSERT(room >= ODP_CONFIG_PACKET_TAILROOM); > +#endif > + seg_data_len = odp_packet_seg_data_len(pkt, segment); > + CU_ASSERT(seg_data_len >= 1); > + /** @todo: should be len - 1 */ > + pull_val = seg_data_len / 2; > + /* Leave one byte in a tailroom for odp_packet_tail() to succeed */ > + push_val = (room > 0) ? room - 1 : room; > + > + _verify_tailroom_shift(pkt, -pull_val); > + _verify_tailroom_shift(pkt, push_val + pull_val); > + _verify_tailroom_shift(pkt, -push_val); > +} > + > +static void packet_segments(void) > +{ > + int num_segs, seg_index; > + uint32_t data_len, buf_len; > + odp_packet_seg_t seg; > + odp_packet_t pkt = test_packet; > + > + CU_ASSERT(odp_packet_is_valid(pkt) == 1); > + > + num_segs = odp_packet_num_segs(pkt); > + CU_ASSERT(num_segs != 0); > + > + if (!odp_packet_is_segmented(pkt)) > + CU_ASSERT(num_segs == 1); > A simpler combined test might be CU_ASSERT(!(odp_packet_is_segmented(pkt) ^ (num_segs > 1))); > + > + seg = odp_packet_first_seg(pkt); > + buf_len = 0; > + data_len = 0; > + seg_index = 0; > + while (seg_index < num_segs && seg != ODP_PACKET_SEG_INVALID) { > + uint32_t seg_data_len, seg_buf_len; > + void *seg_buf_addr, *seg_data; > + > + seg_buf_addr = odp_packet_seg_buf_addr(pkt, seg); > + seg_buf_len = odp_packet_seg_buf_len(pkt, seg); > + seg_data_len = odp_packet_seg_data_len(pkt, seg); > + seg_data = odp_packet_seg_data(pkt, seg); > + > + CU_ASSERT(seg_buf_len > 0); > + CU_ASSERT(seg_data_len > 0); > + CU_ASSERT(seg_buf_len > seg_data_len); > + CU_ASSERT(seg_data != NULL); > + CU_ASSERT(seg_buf_addr != NULL); > + CU_ASSERT(seg_data > seg_buf_addr); > Test should be >= since these will be equal if there is no segment headroom (expected case for today) > + > + buf_len += seg_buf_len; > + data_len += seg_data_len; > + > + /** @todo: touch memory in a segment */ > + seg_index++; > + seg = odp_packet_next_seg(pkt, seg); > + } > + > + CU_ASSERT(seg_index == num_segs); > + CU_ASSERT(buf_len == odp_buffer_size(odp_packet_to_buffer(pkt))); > + CU_ASSERT(data_len == odp_packet_len(pkt)); > + > + if (seg_index == num_segs) > + CU_ASSERT(seg == ODP_PACKET_SEG_INVALID); > +} > + > +static void packet_segment_last(void) > +{ > + odp_packet_t pkt = test_packet; > + odp_packet_seg_t seg; > + > + seg = odp_packet_last_seg(pkt); > + CU_ASSERT_FATAL(seg != ODP_PACKET_SEG_INVALID); > + > + seg = odp_packet_next_seg(pkt, seg); > + CU_ASSERT(seg == ODP_PACKET_SEG_INVALID); > +} > + > +#define TEST_INFLAG(packet, flag) \ > +do { \ > + odp_packet_has_##flag##_set(packet, 0); \ > + CU_ASSERT(odp_packet_has_##flag(packet) == 0); \ > + odp_packet_has_##flag##_set(packet, 1); \ > + CU_ASSERT(odp_packet_has_##flag(packet) == 1); \ > +} while (0) > + > +static void packet_in_flags(void) > +{ > + odp_packet_t pkt = test_packet; > + > + TEST_INFLAG(pkt, l2); > + TEST_INFLAG(pkt, l3); > + TEST_INFLAG(pkt, l4); > + TEST_INFLAG(pkt, eth); > + TEST_INFLAG(pkt, jumbo); > + TEST_INFLAG(pkt, vlan); > + TEST_INFLAG(pkt, vlan_qinq); > + TEST_INFLAG(pkt, arp); > + TEST_INFLAG(pkt, ipv4); > + TEST_INFLAG(pkt, ipv6); > + TEST_INFLAG(pkt, ipfrag); > + TEST_INFLAG(pkt, ipopt); > + TEST_INFLAG(pkt, ipsec); > + TEST_INFLAG(pkt, udp); > + TEST_INFLAG(pkt, tcp); > + TEST_INFLAG(pkt, sctp); > + TEST_INFLAG(pkt, icmp); > +} > + > +static void packet_error_flags(void) > +{ > + odp_packet_t pkt = test_packet; > + int err; > + > + /** > + * The packet have not been classified so it doesn't have error > flag > + * properly set. Just check that function return one of allowed > values. > + * @todo: check classified packet when classifier is added in > place. > + */ > + err = odp_packet_error(pkt); > + CU_ASSERT(err == 0 || err == 1); > + > + err = odp_packet_errflag_frame_len(pkt); > + CU_ASSERT(err == 0 || err == 1); > +} > + > +static void packet_out_flags(void) > +{ > + odp_packet_override_l4_chksum(test_packet); > + CU_PASS("Current API doesn't return any error code\n"); > +} > + > +static void packet_add_rem_data(void) > +{ > + odp_packet_t pkt, new_pkt; > + uint32_t pkt_len, offset, add_len; > + > + pkt = odp_packet_alloc(packet_pool, PACKET_BUF_LEN); > + CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID); > + > + pkt_len = odp_packet_len(pkt); > + /* Insert one more packet length in the middle of a packet */ > + offset = pkt_len / 2; > + add_len = pkt_len; > + > + new_pkt = odp_packet_add_data(pkt, offset, add_len); > + CU_ASSERT(new_pkt != ODP_PACKET_INVALID); > + if (new_pkt == ODP_PACKET_INVALID) > + goto free_packet; > + CU_ASSERT(odp_packet_len(new_pkt) == pkt_len + add_len); > + pkt = new_pkt; > Should probably check that any user context pointer is undisturbed by the operation since if a new pkt is returned the implementation is supposed to copy over the metadata from the original packet. Ideally do this for all of the parser metadata but user context pointer is a good start and arguably the most critical. > + > + pkt_len = odp_packet_len(pkt); > + new_pkt = odp_packet_rem_data(pkt, offset, add_len); > + CU_ASSERT(new_pkt != ODP_PACKET_INVALID); > + if (new_pkt == ODP_PACKET_INVALID) > + goto free_packet; > + CU_ASSERT(odp_packet_len(new_pkt) == pkt_len - add_len); > + pkt = new_pkt; > + > +free_packet: > + odp_packet_free(pkt); > +} > + > + > +#define COMPARE_INFLAG(p1, p2, flag) \ > + CU_ASSERT(odp_packet_has_##flag(p1) == odp_packet_has_##flag(p2)) > + > +static void _packet_compare_inflags(odp_packet_t pkt1, odp_packet_t pkt2) > +{ > + COMPARE_INFLAG(pkt1, pkt2, l2); > + COMPARE_INFLAG(pkt1, pkt2, l3); > + COMPARE_INFLAG(pkt1, pkt2, l4); > + COMPARE_INFLAG(pkt1, pkt2, eth); > + COMPARE_INFLAG(pkt1, pkt2, jumbo); > + COMPARE_INFLAG(pkt1, pkt2, eth); > + COMPARE_INFLAG(pkt1, pkt2, vlan); > + COMPARE_INFLAG(pkt1, pkt2, vlan_qinq); > + COMPARE_INFLAG(pkt1, pkt2, arp); > + COMPARE_INFLAG(pkt1, pkt2, ipv4); > + COMPARE_INFLAG(pkt1, pkt2, ipv6); > + COMPARE_INFLAG(pkt1, pkt2, ipfrag); > + COMPARE_INFLAG(pkt1, pkt2, ipopt); > + COMPARE_INFLAG(pkt1, pkt2, ipsec); > + COMPARE_INFLAG(pkt1, pkt2, udp); > + COMPARE_INFLAG(pkt1, pkt2, tcp); > + COMPARE_INFLAG(pkt1, pkt2, sctp); > + COMPARE_INFLAG(pkt1, pkt2, icmp); > +} > + > +struct pktcmp_ctx { > + odp_packet_t pkt; > + odp_packet_seg_t seg; > + char *ptr; > + uint32_t len; > +}; > + > +static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t pkt2) > +{ > + const int pkt_num = 2; > + int i; > + struct pktcmp_ctx info[pkt_num]; > + uint32_t pkt_len = odp_packet_len(pkt1); > + > + CU_ASSERT_FATAL(pkt_len == odp_packet_len(pkt2)); > + > + info[0].pkt = pkt1; > + info[1].pkt = pkt2; > + > + for (i = 0; i < pkt_num; i++) { > + info[i].seg = odp_packet_first_seg(info[i].pkt); > + CU_ASSERT_FATAL(info[i].seg != ODP_PACKET_SEG_INVALID); > + > + info[i].len = odp_packet_seg_data_len(info[i].pkt, > info[i].seg); > + info[i].ptr = odp_packet_seg_data(info[i].pkt, > info[i].seg); > + CU_ASSERT_FATAL(info[i].len > 0); > + CU_ASSERT_FATAL(info[i].ptr != NULL); > + } > + > + while (pkt_len > 0) { > + uint32_t min_seg_len = (info[0].len < info[1].len) ? > + info[0].len : info[1].len; > + > + CU_ASSERT_FATAL(!memcmp(info[0].ptr, info[1].ptr, > min_seg_len)); > + > + if (pkt_len <= min_seg_len) > + return; > + > + pkt_len -= min_seg_len; > + for (i = 0; i < pkt_num; i++) { > + info[i].len -= min_seg_len; > + if (info[i].len != 0) { > + info[i].ptr += min_seg_len; > + continue; > + } > + > + info[i].seg = odp_packet_next_seg(info[i].pkt, > + info[i].seg); > + CU_ASSERT_FATAL(info[i].seg != > ODP_PACKET_SEG_INVALID); > + > + info[i].len = odp_packet_seg_data_len(info[i].pkt, > + info[i].seg); > + info[i].ptr = odp_packet_seg_data(info[i].pkt, > + info[i].seg); > + CU_ASSERT_FATAL(info[i].len > 0); > + CU_ASSERT_FATAL(info[i].ptr != NULL); > + } > + } > +} > This is a very cumbersome way to accomplish this function. If you want to verify that a packet copy didn't change the packet it's much easier to do that using offsets rather than trying to do it via segments with all of this extra bookkeeping (another reason why applications shouldn't care about how packets are segmented by the underlying implementation). static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t pkt2) { uint32_t len = odp_packet_len(pkt1); uint32_t offset1 = 0, offset2 = 0; uint32_t seglen1, seglen2, cmplen; CU_ASSERT_FATAL(len == odp_packet_len(pkt2)); while (len > 0) { void *pkt1map = odp_packet_offset(pkt1, offset1, &seglen1, NULL); void *pkt2map = odp_packet_offset(pkt2, offset2, &seglen2, NULL)); cmplen = seglen1 < seglen2 ? seglen1 : seglen2; CU_ASSERT(!memcmp(pkt1map, pkt2map, cmplen)); offset1 += cmplen; offset2 += cmplen; len -= cmplen; } } > + > +static void packet_copy(void) > +{ > + odp_packet_t pkt = test_packet; > + odp_packet_t pkt_copy; > + odp_buffer_pool_t pool; > + > + /** @todo: fill original packet with some data */ > + pool = odp_packet_pool(pkt); > + CU_ASSERT_FATAL(pool != ODP_BUFFER_POOL_INVALID); > + pkt_copy = odp_packet_copy(pkt, odp_packet_pool(pkt)); > + CU_ASSERT_FATAL(pkt_copy != ODP_PACKET_INVALID); > + > + CU_ASSERT(odp_packet_len(pkt) == odp_packet_len(pkt_copy)); > + > + _packet_compare_inflags(pkt, pkt_copy); > + _packet_compare_data(pkt, pkt_copy); > + odp_packet_free(pkt_copy); > +} > + > +static void packet_copydata(void) > +{ > + odp_packet_t pkt = test_packet; > + uint32_t pkt_len = odp_packet_len(pkt); > + uint8_t *data_buf; > + uint32_t i; > + int correct_memory; > + > + CU_ASSERT_FATAL(pkt_len > 0); > + > + data_buf = malloc(pkt_len); > + CU_ASSERT_FATAL(data_buf != NULL); > + > + for (i = 0; i < pkt_len; i++) > + data_buf[i] = (uint8_t)i; > + > + CU_ASSERT(!odp_packet_copydata_in(pkt, 0, pkt_len, data_buf)); > + memset(data_buf, 0, pkt_len); > + CU_ASSERT(!odp_packet_copydata_out(pkt, 0, pkt_len, data_buf)); > + > + correct_memory = 1; > + for (i = 0; i < pkt_len; i++) > + if (data_buf[i] != (uint8_t)i) { > + correct_memory = 0; > + break; > + } > + CU_ASSERT(correct_memory); > You can do the verification by copying out from one packet to a memory area then copying in from that same memory area to another packet and then comparing the contents of the original and final packets using _packet_compare_data() shown above. Or you could just trivially mod the above routine to take a memory area as the second argument. > + > + free(data_buf); > +} > + > +static void packet_offset(void) > +{ > + odp_packet_t pkt = test_packet; > + uint32_t seg_len, full_seg_len; > + odp_packet_seg_t seg; > + uint8_t *ptr, *start_ptr; > + uint32_t offset; > + > + ptr = odp_packet_offset(pkt, 0, &seg_len, &seg); > + CU_ASSERT(seg_len > 1); > + CU_ASSERT(seg_len == odp_packet_seg_len(pkt)); > + CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(ptr == odp_packet_data(pkt)); > + CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > + > + /* Query a second byte */ > + start_ptr = ptr; > + full_seg_len = seg_len; > + offset = 1; > + > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(ptr == start_ptr + offset); > + CU_ASSERT(seg_len == full_seg_len - offset); > + > + /* Query the last byte in a segment */ > + offset = full_seg_len - 1; > + > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(ptr == start_ptr + offset); > + CU_ASSERT(seg_len == full_seg_len - offset); > + > + /* Query the last byte in a packet */ > + offset = odp_packet_len(pkt) - 1; > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(seg_len == 1); > + > + /* Pass NULL to [out] arguments */ > + ptr = odp_packet_offset(pkt, 0, NULL, NULL); > + CU_ASSERT(ptr != NULL); > Since we have a fourth argument (not clear what the use case for it is, but it's there) you should include a test to see that it gets back the correct (or at least a non-NULL) segment handle for the specified offset if it's specified. > +} > + > +CU_TestInfo packet_tests[] = { > + _CU_TEST_INFO(packet_alloc_free), > + _CU_TEST_INFO(packet_alloc_segmented), > + _CU_TEST_INFO(packet_basic_metadata), > + _CU_TEST_INFO(packet_debug), > + _CU_TEST_INFO(packet_length), > + _CU_TEST_INFO(packet_headroom), > + _CU_TEST_INFO(packet_tailroom), > + _CU_TEST_INFO(packet_context), > + _CU_TEST_INFO(packet_buffer_conversion), > + _CU_TEST_INFO(packet_layer_offsets), > + _CU_TEST_INFO(packet_segments), > + _CU_TEST_INFO(packet_segment_last), > + _CU_TEST_INFO(packet_in_flags), > + _CU_TEST_INFO(packet_out_flags), > + _CU_TEST_INFO(packet_error_flags), > + _CU_TEST_INFO(packet_add_rem_data), > + _CU_TEST_INFO(packet_copy), > + _CU_TEST_INFO(packet_copydata), > + _CU_TEST_INFO(packet_offset), > + CU_TEST_INFO_NULL, > +}; > diff --git a/test/validation/odp_buffer.c b/test/validation/odp_buffer.c > index 8aa61a9..1102780 100644 > --- a/test/validation/odp_buffer.c > +++ b/test/validation/odp_buffer.c > @@ -15,5 +15,10 @@ CU_SuiteInfo odp_testsuites[] = { > .pInitFunc = buffer_testsuite_init, > .pCleanupFunc = buffer_testsuite_finalize, > }, > + { .pName = "packet tests", > + .pTests = packet_tests, > + .pInitFunc = packet_testsuite_init, > + .pCleanupFunc = packet_testsuite_finalize, > + }, > CU_SUITE_INFO_NULL, > }; > -- > 1.9.1 > >
On 12/18/2014 07:04 PM, Bill Fischofer wrote: > See detailed comments inline. I verified that this passes all tests > when the packet segmentation patch is applied. > > Bill > > On Thu, Dec 18, 2014 at 6:11 AM, Taras Kondratiuk > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> wrote: [snip] > +static void packet_length(void) > +{ > + odp_packet_t pkt = test_packet; > + uint32_t buf_len, headroom, tailroom; > + > + buf_len = odp_packet_buf_len(pkt); > + headroom = odp_packet_headroom(pkt); > + tailroom = odp_packet_tailroom(pkt); > + > + CU_ASSERT(odp_packet_len(pkt) == packet_len); > + CU_ASSERT(headroom >= ODP_CONFIG_PACKET_HEADROOM); > > > As we discussed, should have a guard against ODP_CONFIG_PACKET_HEADROOM > being 0 similar to what you're doing with ODP_CONFIG_PACKET_TAILROOM > since it's legit to specify that config var as 0. Will add. > > > +#if ODP_CONFIG_PACKET_TAILROOM != 0 /* Avoid 'always true' warning */ > + CU_ASSERT(tailroom >= ODP_CONFIG_PACKET_TAILROOM); > +#endif > + CU_ASSERT(buf_len >= packet_len + headroom + tailroom); > +} > + [snip] > +static void _verify_headroom_shift(odp_packet_t packet, > + int shift) > +{ > + uint32_t room = odp_packet_headroom(packet); > + uint32_t seg_data_len = odp_packet_seg_len(packet); > + uint32_t pkt_data_len = odp_packet_len(packet); > + void *data; > + char *data_orig = odp_packet_data(packet); > + char *head_orig = odp_packet_head(packet); > + > + if (shift == 0) > + return; > > > Actually, you should test that 0 does not change things, since a > push/pull of 0 is legitimate. Delete this check, and change test below > to if (shift >= 0). I assumed that push/pull 0 bytes is a programming error, but you are right specification doesn't say anything about it, so I'll remove this guard. > > > + > + if (shift > 0) > + data = odp_packet_push_head(packet, shift); > + else > + data = odp_packet_pull_head(packet, -shift); > + > + CU_ASSERT(data != NULL); > > > Not always true. If shift is out of range NULL is the expected return. Current tests selects push/pull values to be in a valid range. So 'data' should not be NULL. > > > + CU_ASSERT(odp_packet_headroom(packet) == room - shift); > + CU_ASSERT(odp_packet_seg_len(packet) == seg_data_len + shift); > + CU_ASSERT(odp_packet_len(packet) == pkt_data_len + shift); > + CU_ASSERT(odp_packet_data(packet) == data); > > + CU_ASSERT(odp_packet_head(packet) == head_orig); > + CU_ASSERT(data == data_orig - shift); > > > Given the revised definition of push/pull, this test should also verify > that these operations do not change the various l2/l3/l4 offset values. If I'm reading specification correctly, then only odp_packet_push_tail() should preserve offsets: "Handles, pointers and offsets remain valid." Other three functions (head_push/pull, tail_pull) don't promise anything: "User is responsible to update packet meta-data offsets when needed." I'll add check for odp_packet_push_tail(). [snip] > + > +static void packet_segments(void) > +{ > + int num_segs, seg_index; > + uint32_t data_len, buf_len; > + odp_packet_seg_t seg; > + odp_packet_t pkt = test_packet; > + > + CU_ASSERT(odp_packet_is_valid(pkt) == 1); > + > + num_segs = odp_packet_num_segs(pkt); > + CU_ASSERT(num_segs != 0); > + > + if (!odp_packet_is_segmented(pkt)) > + CU_ASSERT(num_segs == 1); > > > A simpler combined test might be > CU_ASSERT(!(odp_packet_is_segmented(pkt) ^ (num_segs > 1))); I'd prefer to leave an initial form and add 'else' branch with CU_ASSERT(num_segs > 1). It is much easier to read and understand what is being checked. [snip] > + > + seg = odp_packet_first_seg(pkt); > + buf_len = 0; > + data_len = 0; > + seg_index = 0; > + while (seg_index < num_segs && seg != ODP_PACKET_SEG_INVALID) { > + uint32_t seg_data_len, seg_buf_len; > + void *seg_buf_addr, *seg_data; > + > + seg_buf_addr = odp_packet_seg_buf_addr(pkt, seg); > + seg_buf_len = odp_packet_seg_buf_len(pkt, seg); > + seg_data_len = odp_packet_seg_data_len(pkt, seg); > + seg_data = odp_packet_seg_data(pkt, seg); > + > + CU_ASSERT(seg_buf_len > 0); > + CU_ASSERT(seg_data_len > 0); > + CU_ASSERT(seg_buf_len > seg_data_len); > + CU_ASSERT(seg_data != NULL); > + CU_ASSERT(seg_buf_addr != NULL); > + CU_ASSERT(seg_data > seg_buf_addr); > > > Test should be >= since these will be equal if there is no segment > headroom (expected case for today) Agree. Then lengths can be also equal a few lines above. [snip] > +static void packet_add_rem_data(void) > +{ > + odp_packet_t pkt, new_pkt; > + uint32_t pkt_len, offset, add_len; > + > + pkt = odp_packet_alloc(packet_pool, PACKET_BUF_LEN); > + CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID); > + > + pkt_len = odp_packet_len(pkt); > + /* Insert one more packet length in the middle of a packet */ > + offset = pkt_len / 2; > + add_len = pkt_len; > + > + new_pkt = odp_packet_add_data(pkt, offset, add_len); > + CU_ASSERT(new_pkt != ODP_PACKET_INVALID); > + if (new_pkt == ODP_PACKET_INVALID) > + goto free_packet; > + CU_ASSERT(odp_packet_len(new_pkt) == pkt_len + add_len); > + pkt = new_pkt; > > > Should probably check that any user context pointer is undisturbed by > the operation since if a new pkt is returned the implementation is > supposed to copy over the metadata from the original packet. Ideally do > this for all of the parser metadata but user context pointer is a good > start and arguably the most critical. Will add. [snip] > +static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t pkt2) > +{ > + const int pkt_num = 2; > + int i; > + struct pktcmp_ctx info[pkt_num]; > + uint32_t pkt_len = odp_packet_len(pkt1); > + > + CU_ASSERT_FATAL(pkt_len == odp_packet_len(pkt2)); > + > + info[0].pkt = pkt1; > + info[1].pkt = pkt2; > + > + for (i = 0; i < pkt_num; i++) { > + info[i].seg = odp_packet_first_seg(info[i].pkt); > + CU_ASSERT_FATAL(info[i].seg != ODP_PACKET_SEG_INVALID); > + > + info[i].len = odp_packet_seg_data_len(info[i].pkt, > info[i].seg); > + info[i].ptr = odp_packet_seg_data(info[i].pkt, > info[i].seg); > + CU_ASSERT_FATAL(info[i].len > 0); > + CU_ASSERT_FATAL(info[i].ptr != NULL); > + } > + > + while (pkt_len > 0) { > + uint32_t min_seg_len = (info[0].len < info[1].len) ? > + info[0].len : info[1].len; > + > + CU_ASSERT_FATAL(!memcmp(info[0].ptr, info[1].ptr, > min_seg_len)); > + > + if (pkt_len <= min_seg_len) > + return; > + > + pkt_len -= min_seg_len; > + for (i = 0; i < pkt_num; i++) { > + info[i].len -= min_seg_len; > + if (info[i].len != 0) { > + info[i].ptr += min_seg_len; > + continue; > + } > + > + info[i].seg = odp_packet_next_seg(info[i].pkt, > + info[i].seg); > + CU_ASSERT_FATAL(info[i].seg != > ODP_PACKET_SEG_INVALID); > + > + info[i].len = > odp_packet_seg_data_len(info[i].pkt, > + > info[i].seg); > + info[i].ptr = odp_packet_seg_data(info[i].pkt, > + info[i].seg); > + CU_ASSERT_FATAL(info[i].len > 0); > + CU_ASSERT_FATAL(info[i].ptr != NULL); > + } > + } > +} > > > This is a very cumbersome way to accomplish this function. If you want > to verify that a packet copy didn't change the packet it's much easier > to do that using offsets rather than trying to do it via segments with > all of this extra bookkeeping (another reason why applications shouldn't > care about how packets are segmented by the underlying implementation). > > static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t pkt2) > { > uint32_t len = odp_packet_len(pkt1); > uint32_t offset1 = 0, offset2 = 0; > uint32_t seglen1, seglen2, cmplen; > > CU_ASSERT_FATAL(len == odp_packet_len(pkt2)); > > while (len > 0) { > void *pkt1map = odp_packet_offset(pkt1, offset1, &seglen1, NULL); > void *pkt2map = odp_packet_offset(pkt2, offset2, &seglen2, > NULL)); > > cmplen = seglen1 < seglen2 ? seglen1 : seglen2; > CU_ASSERT(!memcmp(pkt1map, pkt2map, cmplen)); > > offset1 += cmplen; > offset2 += cmplen; > len -= cmplen; > } > > } Agree, using offsets is much easier. It can be even simpler: only one offset variable is sufficient. odp_packet_offset() was the last API I wrote test for, so I didn't have it in mind when I wrote this one. [snip] > + > +static void packet_copydata(void) > +{ > + odp_packet_t pkt = test_packet; > + uint32_t pkt_len = odp_packet_len(pkt); > + uint8_t *data_buf; > + uint32_t i; > + int correct_memory; > + > + CU_ASSERT_FATAL(pkt_len > 0); > + > + data_buf = malloc(pkt_len); > + CU_ASSERT_FATAL(data_buf != NULL); > + > + for (i = 0; i < pkt_len; i++) > + data_buf[i] = (uint8_t)i; > + > + CU_ASSERT(!odp_packet_copydata_in(pkt, 0, pkt_len, data_buf)); > + memset(data_buf, 0, pkt_len); > + CU_ASSERT(!odp_packet_copydata_out(pkt, 0, pkt_len, data_buf)); > + > + correct_memory = 1; > + for (i = 0; i < pkt_len; i++) > + if (data_buf[i] != (uint8_t)i) { > + correct_memory = 0; > + break; > + } > + CU_ASSERT(correct_memory); > > > You can do the verification by copying out from one packet to a memory > area then copying in from that same memory area to another packet and > then comparing the contents of the original and final packets using > _packet_compare_data() shown above. Or you could just trivially mod the > above routine to take a memory area as the second argument. What will be the advantage of this change? It will involve other API to this test (odp_packet_offset()) which is not desirable. [snip] > > > + > + free(data_buf); > +} > + > +static void packet_offset(void) > +{ > + odp_packet_t pkt = test_packet; > + uint32_t seg_len, full_seg_len; > + odp_packet_seg_t seg; > + uint8_t *ptr, *start_ptr; > + uint32_t offset; > + > + ptr = odp_packet_offset(pkt, 0, &seg_len, &seg); > + CU_ASSERT(seg_len > 1); > + CU_ASSERT(seg_len == odp_packet_seg_len(pkt)); > + CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(ptr == odp_packet_data(pkt)); > + CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > + > + /* Query a second byte */ > + start_ptr = ptr; > + full_seg_len = seg_len; > + offset = 1; > + > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(ptr == start_ptr + offset); > + CU_ASSERT(seg_len == full_seg_len - offset); > + > + /* Query the last byte in a segment */ > + offset = full_seg_len - 1; > + > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(ptr == start_ptr + offset); > + CU_ASSERT(seg_len == full_seg_len - offset); > + > + /* Query the last byte in a packet */ > + offset = odp_packet_len(pkt) - 1; > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > + CU_ASSERT(ptr != NULL); > + CU_ASSERT(seg_len == 1); > + > + /* Pass NULL to [out] arguments */ > + ptr = odp_packet_offset(pkt, 0, NULL, NULL); > + CU_ASSERT(ptr != NULL); > > > Since we have a fourth argument (not clear what the use case for it is, > but it's there) you should include a test to see that it gets back the > correct (or at least a non-NULL) segment handle for the specified offset > if it's specified. Forth argument is used to get segment length and data pointer above. Will add a check for invalid segment there.
On Fri, Dec 19, 2014 at 8:46 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > > On 12/18/2014 07:04 PM, Bill Fischofer wrote: > > See detailed comments inline. I verified that this passes all tests > > when the packet segmentation patch is applied. > > > > Bill > > > > On Thu, Dec 18, 2014 at 6:11 AM, Taras Kondratiuk > > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> > wrote: > > [snip] > > > +static void packet_length(void) > > +{ > > + odp_packet_t pkt = test_packet; > > + uint32_t buf_len, headroom, tailroom; > > + > > + buf_len = odp_packet_buf_len(pkt); > > + headroom = odp_packet_headroom(pkt); > > + tailroom = odp_packet_tailroom(pkt); > > + > > + CU_ASSERT(odp_packet_len(pkt) == packet_len); > > + CU_ASSERT(headroom >= ODP_CONFIG_PACKET_HEADROOM); > > > > > > As we discussed, should have a guard against ODP_CONFIG_PACKET_HEADROOM > > being 0 similar to what you're doing with ODP_CONFIG_PACKET_TAILROOM > > since it's legit to specify that config var as 0. > > Will add. > > > > > > > +#if ODP_CONFIG_PACKET_TAILROOM != 0 /* Avoid 'always true' warning > */ > > + CU_ASSERT(tailroom >= ODP_CONFIG_PACKET_TAILROOM); > > +#endif > > + CU_ASSERT(buf_len >= packet_len + headroom + tailroom); > > +} > > + > > [snip] > > > +static void _verify_headroom_shift(odp_packet_t packet, > > + int shift) > > +{ > > + uint32_t room = odp_packet_headroom(packet); > > + uint32_t seg_data_len = odp_packet_seg_len(packet); > > + uint32_t pkt_data_len = odp_packet_len(packet); > > + void *data; > > + char *data_orig = odp_packet_data(packet); > > + char *head_orig = odp_packet_head(packet); > > + > > + if (shift == 0) > > + return; > > > > > > Actually, you should test that 0 does not change things, since a > > push/pull of 0 is legitimate. Delete this check, and change test below > > to if (shift >= 0). > > I assumed that push/pull 0 bytes is a programming error, but you are > right specification doesn't say anything about it, so I'll remove this > guard. > > > > > > > + > > + if (shift > 0) > > + data = odp_packet_push_head(packet, shift); > > + else > > + data = odp_packet_pull_head(packet, -shift); > > + > > + CU_ASSERT(data != NULL); > > > > > > Not always true. If shift is out of range NULL is the expected return. > > Current tests selects push/pull values to be in a valid range. So > 'data' should not be NULL. > > Ok, however I assume at some point we'll add negative tests in which case this will resurface. > > > > > > + CU_ASSERT(odp_packet_headroom(packet) == room - shift); > > + CU_ASSERT(odp_packet_seg_len(packet) == seg_data_len + > shift); > > + CU_ASSERT(odp_packet_len(packet) == pkt_data_len + shift); > > + CU_ASSERT(odp_packet_data(packet) == data); > > > > + CU_ASSERT(odp_packet_head(packet) == head_orig); > > + CU_ASSERT(data == data_orig - shift); > > > > > > Given the revised definition of push/pull, this test should also verify > > that these operations do not change the various l2/l3/l4 offset values. > > If I'm reading specification correctly, then only > odp_packet_push_tail() should preserve offsets: > "Handles, pointers and offsets remain valid." > No, the change we made is to ensure that push/pull operations on the head do not affect the various offsets. If the spec said nothing about that then the original code that adjusted them would have been correct. Per the revised spec, it is an error for the implementation to change these values as part of those operations, so the test should verify that. > > Other three functions (head_push/pull, tail_pull) don't promise > anything: > "User is responsible to update packet meta-data offsets when needed." > This means that the implementation MUST NOT change these as part of a head push/pull. So tests should verify that stipulation. > I'll add check for odp_packet_push_tail(). > > [snip] > > > + > > +static void packet_segments(void) > > +{ > > + int num_segs, seg_index; > > + uint32_t data_len, buf_len; > > + odp_packet_seg_t seg; > > + odp_packet_t pkt = test_packet; > > + > > + CU_ASSERT(odp_packet_is_valid(pkt) == 1); > > + > > + num_segs = odp_packet_num_segs(pkt); > > + CU_ASSERT(num_segs != 0); > > + > > + if (!odp_packet_is_segmented(pkt)) > > + CU_ASSERT(num_segs == 1); > > > > > > A simpler combined test might be > > CU_ASSERT(!(odp_packet_is_segmented(pkt) ^ (num_segs > 1))); > > I'd prefer to leave an initial form and add 'else' branch with > CU_ASSERT(num_segs > 1). It is much easier to read and understand what > is being checked. > No problem with that. > > [snip] > > > + > > + seg = odp_packet_first_seg(pkt); > > + buf_len = 0; > > + data_len = 0; > > + seg_index = 0; > > + while (seg_index < num_segs && seg != > ODP_PACKET_SEG_INVALID) { > > + uint32_t seg_data_len, seg_buf_len; > > + void *seg_buf_addr, *seg_data; > > + > > + seg_buf_addr = odp_packet_seg_buf_addr(pkt, seg); > > + seg_buf_len = odp_packet_seg_buf_len(pkt, seg); > > + seg_data_len = odp_packet_seg_data_len(pkt, seg); > > + seg_data = odp_packet_seg_data(pkt, seg); > > + > > + CU_ASSERT(seg_buf_len > 0); > > + CU_ASSERT(seg_data_len > 0); > > + CU_ASSERT(seg_buf_len > seg_data_len); > > + CU_ASSERT(seg_data != NULL); > > + CU_ASSERT(seg_buf_addr != NULL); > > + CU_ASSERT(seg_data > seg_buf_addr); > > > > > > Test should be >= since these will be equal if there is no segment > > headroom (expected case for today) > > Agree. Then lengths can be also equal a few lines above. > > [snip] > > > +static void packet_add_rem_data(void) > > +{ > > + odp_packet_t pkt, new_pkt; > > + uint32_t pkt_len, offset, add_len; > > + > > + pkt = odp_packet_alloc(packet_pool, PACKET_BUF_LEN); > > + CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID); > > + > > + pkt_len = odp_packet_len(pkt); > > + /* Insert one more packet length in the middle of a packet */ > > + offset = pkt_len / 2; > > + add_len = pkt_len; > > + > > + new_pkt = odp_packet_add_data(pkt, offset, add_len); > > + CU_ASSERT(new_pkt != ODP_PACKET_INVALID); > > + if (new_pkt == ODP_PACKET_INVALID) > > + goto free_packet; > > + CU_ASSERT(odp_packet_len(new_pkt) == pkt_len + add_len); > > + pkt = new_pkt; > > > > > > Should probably check that any user context pointer is undisturbed by > > the operation since if a new pkt is returned the implementation is > > supposed to copy over the metadata from the original packet. Ideally do > > this for all of the parser metadata but user context pointer is a good > > start and arguably the most critical. > > Will add. > > [snip] > > > +static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t > pkt2) > > +{ > > + const int pkt_num = 2; > > + int i; > > + struct pktcmp_ctx info[pkt_num]; > > + uint32_t pkt_len = odp_packet_len(pkt1); > > + > > + CU_ASSERT_FATAL(pkt_len == odp_packet_len(pkt2)); > > + > > + info[0].pkt = pkt1; > > + info[1].pkt = pkt2; > > + > > + for (i = 0; i < pkt_num; i++) { > > + info[i].seg = odp_packet_first_seg(info[i].pkt); > > + CU_ASSERT_FATAL(info[i].seg != > ODP_PACKET_SEG_INVALID); > > + > > + info[i].len = odp_packet_seg_data_len(info[i].pkt, > > info[i].seg); > > + info[i].ptr = odp_packet_seg_data(info[i].pkt, > > info[i].seg); > > + CU_ASSERT_FATAL(info[i].len > 0); > > + CU_ASSERT_FATAL(info[i].ptr != NULL); > > + } > > + > > + while (pkt_len > 0) { > > + uint32_t min_seg_len = (info[0].len < info[1].len) ? > > + info[0].len : info[1].len; > > + > > + CU_ASSERT_FATAL(!memcmp(info[0].ptr, info[1].ptr, > > min_seg_len)); > > + > > + if (pkt_len <= min_seg_len) > > + return; > > + > > + pkt_len -= min_seg_len; > > + for (i = 0; i < pkt_num; i++) { > > + info[i].len -= min_seg_len; > > + if (info[i].len != 0) { > > + info[i].ptr += min_seg_len; > > + continue; > > + } > > + > > + info[i].seg = > odp_packet_next_seg(info[i].pkt, > > + > info[i].seg); > > + CU_ASSERT_FATAL(info[i].seg != > > ODP_PACKET_SEG_INVALID); > > + > > + info[i].len = > > odp_packet_seg_data_len(info[i].pkt, > > + > > info[i].seg); > > + info[i].ptr = > odp_packet_seg_data(info[i].pkt, > > + > info[i].seg); > > + CU_ASSERT_FATAL(info[i].len > 0); > > + CU_ASSERT_FATAL(info[i].ptr != NULL); > > + } > > + } > > +} > > > > > > This is a very cumbersome way to accomplish this function. If you want > > to verify that a packet copy didn't change the packet it's much easier > > to do that using offsets rather than trying to do it via segments with > > all of this extra bookkeeping (another reason why applications shouldn't > > care about how packets are segmented by the underlying implementation). > > > > static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t pkt2) > > { > > uint32_t len = odp_packet_len(pkt1); > > uint32_t offset1 = 0, offset2 = 0; > > uint32_t seglen1, seglen2, cmplen; > > > > CU_ASSERT_FATAL(len == odp_packet_len(pkt2)); > > > > while (len > 0) { > > void *pkt1map = odp_packet_offset(pkt1, offset1, &seglen1, > NULL); > > void *pkt2map = odp_packet_offset(pkt2, offset2, &seglen2, > > NULL)); > > > > cmplen = seglen1 < seglen2 ? seglen1 : seglen2; > > CU_ASSERT(!memcmp(pkt1map, pkt2map, cmplen)); > > > > offset1 += cmplen; > > offset2 += cmplen; > > len -= cmplen; > > } > > > > } > > Agree, using offsets is much easier. It can be even simpler: only one > offset variable is sufficient. > odp_packet_offset() was the last API I wrote test for, so I didn't have > it in mind when I wrote this one. > You need two offsets because there is no guarantee that the two packets are segmented the same way. They could have different numbers of segments or the segment sizes could vary between them. There's no requirement in the spec that segments are of uniform size. > > [snip] > > > + > > +static void packet_copydata(void) > > +{ > > + odp_packet_t pkt = test_packet; > > + uint32_t pkt_len = odp_packet_len(pkt); > > + uint8_t *data_buf; > > + uint32_t i; > > + int correct_memory; > > + > > + CU_ASSERT_FATAL(pkt_len > 0); > > + > > + data_buf = malloc(pkt_len); > > + CU_ASSERT_FATAL(data_buf != NULL); > > + > > + for (i = 0; i < pkt_len; i++) > > + data_buf[i] = (uint8_t)i; > > + > > + CU_ASSERT(!odp_packet_copydata_in(pkt, 0, pkt_len, > data_buf)); > > + memset(data_buf, 0, pkt_len); > > + CU_ASSERT(!odp_packet_copydata_out(pkt, 0, pkt_len, > data_buf)); > > + > > + correct_memory = 1; > > + for (i = 0; i < pkt_len; i++) > > + if (data_buf[i] != (uint8_t)i) { > > + correct_memory = 0; > > + break; > > + } > > + CU_ASSERT(correct_memory); > > > > > > You can do the verification by copying out from one packet to a memory > > area then copying in from that same memory area to another packet and > > then comparing the contents of the original and final packets using > > _packet_compare_data() shown above. Or you could just trivially mod the > > above routine to take a memory area as the second argument. > > What will be the advantage of this change? > It will involve other API to this test (odp_packet_offset()) which > is not desirable. > OK. Nothing wrong with this test as written. > > [snip] > > > > > > > + > > + free(data_buf); > > +} > > + > > +static void packet_offset(void) > > +{ > > + odp_packet_t pkt = test_packet; > > + uint32_t seg_len, full_seg_len; > > + odp_packet_seg_t seg; > > + uint8_t *ptr, *start_ptr; > > + uint32_t offset; > > + > > + ptr = odp_packet_offset(pkt, 0, &seg_len, &seg); > > + CU_ASSERT(seg_len > 1); > > + CU_ASSERT(seg_len == odp_packet_seg_len(pkt)); > > + CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(ptr == odp_packet_data(pkt)); > > + CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > > + > > + /* Query a second byte */ > > + start_ptr = ptr; > > + full_seg_len = seg_len; > > + offset = 1; > > + > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(ptr == start_ptr + offset); > > + CU_ASSERT(seg_len == full_seg_len - offset); > > + > > + /* Query the last byte in a segment */ > > + offset = full_seg_len - 1; > > + > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(ptr == start_ptr + offset); > > + CU_ASSERT(seg_len == full_seg_len - offset); > > + > > + /* Query the last byte in a packet */ > > + offset = odp_packet_len(pkt) - 1; > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(seg_len == 1); > > + > > + /* Pass NULL to [out] arguments */ > > + ptr = odp_packet_offset(pkt, 0, NULL, NULL); > > + CU_ASSERT(ptr != NULL); > > > > > > Since we have a fourth argument (not clear what the use case for it is, > > but it's there) you should include a test to see that it gets back the > > correct (or at least a non-NULL) segment handle for the specified offset > > if it's specified. > > Forth argument is used to get segment length and data pointer above. > Will add a check for invalid segment there. > No, the fourth argument returns the segment handle (odp_packet_seg_t) of the segment that contains the requested offset. This is not a usable address by itself. It's not clear how an application would use it, but it's part of the spec. > > -- > Taras Kondratiuk >
On 12/19/2014 05:38 PM, Bill Fischofer wrote: > > > On Fri, Dec 19, 2014 at 8:46 AM, Taras Kondratiuk > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> wrote: > > > > > > > + > > + if (shift > 0) > > + data = odp_packet_push_head(packet, shift); > > + else > > + data = odp_packet_pull_head(packet, -shift); > > + > > + CU_ASSERT(data != NULL); > > > > > > Not always true. If shift is out of range NULL is the expected return. > > Current tests selects push/pull values to be in a valid range. So > 'data' should not be NULL. > > > Ok, however I assume at some point we'll add negative tests in which > case this will resurface. Right we will add negative tests, but they won't use this function. > > + CU_ASSERT(odp_packet_headroom(packet) == room - shift); > > + CU_ASSERT(odp_packet_seg_len(packet) == seg_data_len + shift); > > + CU_ASSERT(odp_packet_len(packet) == pkt_data_len + shift); > > + CU_ASSERT(odp_packet_data(packet) == data); > > > > + CU_ASSERT(odp_packet_head(packet) == head_orig); > > + CU_ASSERT(data == data_orig - shift); > > > > > > Given the revised definition of push/pull, this test should also verify > > that these operations do not change the various l2/l3/l4 offset values. > > If I'm reading specification correctly, then only > odp_packet_push_tail() should preserve offsets: > "Handles, pointers and offsets remain valid." > > > No, the change we made is to ensure that push/pull operations on the > head do not affect the various offsets. If the spec said nothing about > that then the original code that adjusted them would have been correct. > Per the revised spec, it is an error for the implementation to change > these values as part of those operations, so the test should verify that. > > > > Other three functions (head_push/pull, tail_pull) don't promise > anything: > "User is responsible to update packet meta-data offsets when needed." > > > This means that the implementation MUST NOT change these as part of a > head push/pull. So tests should verify that stipulation. But the spec doesn't say that. I read it like: offsets are not valid and user is responsible to update them. I think we need clarification from Petri. > > +static void _packet_compare_data(odp_packet_t pkt1, > odp_packet_t pkt2) > > +{ > > + const int pkt_num = 2; > > + int i; > > + struct pktcmp_ctx info[pkt_num]; > > + uint32_t pkt_len = odp_packet_len(pkt1); > > + > > + CU_ASSERT_FATAL(pkt_len == odp_packet_len(pkt2)); > > + > > + info[0].pkt = pkt1; > > + info[1].pkt = pkt2; > > + > > + for (i = 0; i < pkt_num; i++) { > > + info[i].seg = odp_packet_first_seg(info[i].pkt); > > + CU_ASSERT_FATAL(info[i].seg != > ODP_PACKET_SEG_INVALID); > > + > > + info[i].len = odp_packet_seg_data_len(info[i].pkt, > > info[i].seg); > > + info[i].ptr = odp_packet_seg_data(info[i].pkt, > > info[i].seg); > > + CU_ASSERT_FATAL(info[i].len > 0); > > + CU_ASSERT_FATAL(info[i].ptr != NULL); > > + } > > + > > + while (pkt_len > 0) { > > + uint32_t min_seg_len = (info[0].len < > info[1].len) ? > > + info[0].len : info[1].len; > > + > > + CU_ASSERT_FATAL(!memcmp(info[0].ptr, info[1].ptr, > > min_seg_len)); > > + > > + if (pkt_len <= min_seg_len) > > + return; > > + > > + pkt_len -= min_seg_len; > > + for (i = 0; i < pkt_num; i++) { > > + info[i].len -= min_seg_len; > > + if (info[i].len != 0) { > > + info[i].ptr += min_seg_len; > > + continue; > > + } > > + > > + info[i].seg = > odp_packet_next_seg(info[i].pkt, > > + > info[i].seg); > > + CU_ASSERT_FATAL(info[i].seg != > > ODP_PACKET_SEG_INVALID); > > + > > + info[i].len = > > odp_packet_seg_data_len(info[i].pkt, > > + > > info[i].seg); > > + info[i].ptr = > odp_packet_seg_data(info[i].pkt, > > + > info[i].seg); > > + CU_ASSERT_FATAL(info[i].len > 0); > > + CU_ASSERT_FATAL(info[i].ptr != NULL); > > + } > > + } > > +} > > > > > > This is a very cumbersome way to accomplish this function. If you > want > > to verify that a packet copy didn't change the packet it's much easier > > to do that using offsets rather than trying to do it via segments with > > all of this extra bookkeeping (another reason why applications > shouldn't > > care about how packets are segmented by the underlying > implementation). > > > > static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t pkt2) > > { > > uint32_t len = odp_packet_len(pkt1); > > uint32_t offset1 = 0, offset2 = 0; > > uint32_t seglen1, seglen2, cmplen; > > > > CU_ASSERT_FATAL(len == odp_packet_len(pkt2)); > > > > while (len > 0) { > > void *pkt1map = odp_packet_offset(pkt1, offset1, > &seglen1, NULL); > > void *pkt2map = odp_packet_offset(pkt2, offset2, &seglen2, > > NULL)); > > > > cmplen = seglen1 < seglen2 ? seglen1 : seglen2; > > CU_ASSERT(!memcmp(pkt1map, pkt2map, cmplen)); > > > > offset1 += cmplen; > > offset2 += cmplen; > > len -= cmplen; > > } > > > > } > > Agree, using offsets is much easier. It can be even simpler: only one > offset variable is sufficient. > odp_packet_offset() was the last API I wrote test for, so I didn't have > it in mind when I wrote this one. > > > You need two offsets because there is no guarantee that the two packets > are segmented the same way. They could have different numbers of > segments or the segment sizes could vary between them. There's no > requirement in the spec that segments are of uniform size. Offset doesn't depend on segmentation and the same offset should point to the same data in both packets. Right? > > + > > + free(data_buf); > > +} > > + > > +static void packet_offset(void) > > +{ > > + odp_packet_t pkt = test_packet; > > + uint32_t seg_len, full_seg_len; > > + odp_packet_seg_t seg; > > + uint8_t *ptr, *start_ptr; > > + uint32_t offset; > > + > > + ptr = odp_packet_offset(pkt, 0, &seg_len, &seg); > > + CU_ASSERT(seg_len > 1); > > + CU_ASSERT(seg_len == odp_packet_seg_len(pkt)); > > + CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(ptr == odp_packet_data(pkt)); > > + CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > > + > > + /* Query a second byte */ > > + start_ptr = ptr; > > + full_seg_len = seg_len; > > + offset = 1; > > + > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(ptr == start_ptr + offset); > > + CU_ASSERT(seg_len == full_seg_len - offset); > > + > > + /* Query the last byte in a segment */ > > + offset = full_seg_len - 1; > > + > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(ptr == start_ptr + offset); > > + CU_ASSERT(seg_len == full_seg_len - offset); > > + > > + /* Query the last byte in a packet */ > > + offset = odp_packet_len(pkt) - 1; > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > + CU_ASSERT(ptr != NULL); > > + CU_ASSERT(seg_len == 1); > > + > > + /* Pass NULL to [out] arguments */ > > + ptr = odp_packet_offset(pkt, 0, NULL, NULL); > > + CU_ASSERT(ptr != NULL); > > > > > > Since we have a fourth argument (not clear what the use case for > it is, > > but it's there) you should include a test to see that it gets back the > > correct (or at least a non-NULL) segment handle for the specified > offset > > if it's specified. > > Forth argument is used to get segment length and data pointer above. > Will add a check for invalid segment there. > > > No, the fourth argument returns the segment handle (odp_packet_seg_t) of > the segment that contains the requested offset. This is not a usable > address by itself. It's not clear how an application would use it, but > it's part of the spec. I'm not sure I've got you point. Fourth argument is a segment handle and I use it with segment API: CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg));
On Fri, Dec 19, 2014 at 10:34 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > > On 12/19/2014 05:38 PM, Bill Fischofer wrote: > > > > > > On Fri, Dec 19, 2014 at 8:46 AM, Taras Kondratiuk > > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> > wrote: > > > > > > > > > > > > + > > > + if (shift > 0) > > > + data = odp_packet_push_head(packet, shift); > > > + else > > > + data = odp_packet_pull_head(packet, -shift); > > > + > > > + CU_ASSERT(data != NULL); > > > > > > > > > Not always true. If shift is out of range NULL is the expected > return. > > > > Current tests selects push/pull values to be in a valid range. So > > 'data' should not be NULL. > > > > > > Ok, however I assume at some point we'll add negative tests in which > > case this will resurface. > > Right we will add negative tests, but they won't use this function. > > > > > + CU_ASSERT(odp_packet_headroom(packet) == room - shift); > > > + CU_ASSERT(odp_packet_seg_len(packet) == seg_data_len + > shift); > > > + CU_ASSERT(odp_packet_len(packet) == pkt_data_len + > shift); > > > + CU_ASSERT(odp_packet_data(packet) == data); > > > > > > + CU_ASSERT(odp_packet_head(packet) == head_orig); > > > + CU_ASSERT(data == data_orig - shift); > > > > > > > > > Given the revised definition of push/pull, this test should also > verify > > > that these operations do not change the various l2/l3/l4 offset > values. > > > > If I'm reading specification correctly, then only > > odp_packet_push_tail() should preserve offsets: > > "Handles, pointers and offsets remain valid." > > > > > > No, the change we made is to ensure that push/pull operations on the > > head do not affect the various offsets. If the spec said nothing about > > that then the original code that adjusted them would have been correct. > > Per the revised spec, it is an error for the implementation to change > > these values as part of those operations, so the test should verify that. > > > > > > > > Other three functions (head_push/pull, tail_pull) don't promise > > anything: > > "User is responsible to update packet meta-data offsets when needed." > > > > > > This means that the implementation MUST NOT change these as part of a > > head push/pull. So tests should verify that stipulation. > > But the spec doesn't say that. I read it like: offsets are not valid > and user is responsible to update them. > I think we need clarification from Petri. If the spec was silent on this point then the original implementation which adjusted them would be conformant. If the spec said "these offsets are indeterminate following a push/pull" then again the original implementation would be conformant. The only reason for changing the code was to bring it into conformance with the spec, so it is definitely now part of the spec that these offsets MUST NOT be changed as part of push/pull operations. So this needs to be part of the test. > > > > +static void _packet_compare_data(odp_packet_t pkt1, > > odp_packet_t pkt2) > > > +{ > > > + const int pkt_num = 2; > > > + int i; > > > + struct pktcmp_ctx info[pkt_num]; > > > + uint32_t pkt_len = odp_packet_len(pkt1); > > > + > > > + CU_ASSERT_FATAL(pkt_len == odp_packet_len(pkt2)); > > > + > > > + info[0].pkt = pkt1; > > > + info[1].pkt = pkt2; > > > + > > > + for (i = 0; i < pkt_num; i++) { > > > + info[i].seg = > odp_packet_first_seg(info[i].pkt); > > > + CU_ASSERT_FATAL(info[i].seg != > > ODP_PACKET_SEG_INVALID); > > > + > > > + info[i].len = > odp_packet_seg_data_len(info[i].pkt, > > > info[i].seg); > > > + info[i].ptr = odp_packet_seg_data(info[i].pkt, > > > info[i].seg); > > > + CU_ASSERT_FATAL(info[i].len > 0); > > > + CU_ASSERT_FATAL(info[i].ptr != NULL); > > > + } > > > + > > > + while (pkt_len > 0) { > > > + uint32_t min_seg_len = (info[0].len < > > info[1].len) ? > > > + info[0].len : info[1].len; > > > + > > > + CU_ASSERT_FATAL(!memcmp(info[0].ptr, > info[1].ptr, > > > min_seg_len)); > > > + > > > + if (pkt_len <= min_seg_len) > > > + return; > > > + > > > + pkt_len -= min_seg_len; > > > + for (i = 0; i < pkt_num; i++) { > > > + info[i].len -= min_seg_len; > > > + if (info[i].len != 0) { > > > + info[i].ptr += min_seg_len; > > > + continue; > > > + } > > > + > > > + info[i].seg = > > odp_packet_next_seg(info[i].pkt, > > > + > > info[i].seg); > > > + CU_ASSERT_FATAL(info[i].seg != > > > ODP_PACKET_SEG_INVALID); > > > + > > > + info[i].len = > > > odp_packet_seg_data_len(info[i].pkt, > > > + > > > info[i].seg); > > > + info[i].ptr = > > odp_packet_seg_data(info[i].pkt, > > > + > > info[i].seg); > > > + CU_ASSERT_FATAL(info[i].len > 0); > > > + CU_ASSERT_FATAL(info[i].ptr != NULL); > > > + } > > > + } > > > +} > > > > > > > > > This is a very cumbersome way to accomplish this function. If you > > want > > > to verify that a packet copy didn't change the packet it's much > easier > > > to do that using offsets rather than trying to do it via segments > with > > > all of this extra bookkeeping (another reason why applications > > shouldn't > > > care about how packets are segmented by the underlying > > implementation). > > > > > > static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t > pkt2) > > > { > > > uint32_t len = odp_packet_len(pkt1); > > > uint32_t offset1 = 0, offset2 = 0; > > > uint32_t seglen1, seglen2, cmplen; > > > > > > CU_ASSERT_FATAL(len == odp_packet_len(pkt2)); > > > > > > while (len > 0) { > > > void *pkt1map = odp_packet_offset(pkt1, offset1, > > &seglen1, NULL); > > > void *pkt2map = odp_packet_offset(pkt2, offset2, > &seglen2, > > > NULL)); > > > > > > cmplen = seglen1 < seglen2 ? seglen1 : seglen2; > > > CU_ASSERT(!memcmp(pkt1map, pkt2map, cmplen)); > > > > > > offset1 += cmplen; > > > offset2 += cmplen; > > > len -= cmplen; > > > } > > > > > > } > > > > Agree, using offsets is much easier. It can be even simpler: only one > > offset variable is sufficient. > > odp_packet_offset() was the last API I wrote test for, so I didn't > have > > it in mind when I wrote this one. > > > > > > You need two offsets because there is no guarantee that the two packets > > are segmented the same way. They could have different numbers of > > segments or the segment sizes could vary between them. There's no > > requirement in the spec that segments are of uniform size. > > Offset doesn't depend on segmentation and the same offset > should point to the same data in both packets. Right? > OK, I see what you're now saying. Yes, you can have a single offset that starts at 0 and is incremented each iteration by cmplen. So that's a good simplification. > > > > > + > > > + free(data_buf); > > > +} > > > + > > > +static void packet_offset(void) > > > +{ > > > + odp_packet_t pkt = test_packet; > > > + uint32_t seg_len, full_seg_len; > > > + odp_packet_seg_t seg; > > > + uint8_t *ptr, *start_ptr; > > > + uint32_t offset; > > > + > > > + ptr = odp_packet_offset(pkt, 0, &seg_len, &seg); > > > + CU_ASSERT(seg_len > 1); > > > + CU_ASSERT(seg_len == odp_packet_seg_len(pkt)); > > > + CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, > seg)); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(ptr == odp_packet_data(pkt)); > > > + CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > > > + > > > + /* Query a second byte */ > > > + start_ptr = ptr; > > > + full_seg_len = seg_len; > > > + offset = 1; > > > + > > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(ptr == start_ptr + offset); > > > + CU_ASSERT(seg_len == full_seg_len - offset); > > > + > > > + /* Query the last byte in a segment */ > > > + offset = full_seg_len - 1; > > > + > > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(ptr == start_ptr + offset); > > > + CU_ASSERT(seg_len == full_seg_len - offset); > > > + > > > + /* Query the last byte in a packet */ > > > + offset = odp_packet_len(pkt) - 1; > > > + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(seg_len == 1); > > > + > > > + /* Pass NULL to [out] arguments */ > > > + ptr = odp_packet_offset(pkt, 0, NULL, NULL); > > > + CU_ASSERT(ptr != NULL); > > > > > > > > > Since we have a fourth argument (not clear what the use case for > > it is, > > > but it's there) you should include a test to see that it gets back > the > > > correct (or at least a non-NULL) segment handle for the specified > > offset > > > if it's specified. > > > > Forth argument is used to get segment length and data pointer above. > > Will add a check for invalid segment there. > > > > > > No, the fourth argument returns the segment handle (odp_packet_seg_t) of > > the segment that contains the requested offset. This is not a usable > > address by itself. It's not clear how an application would use it, but > > it's part of the spec. > > I'm not sure I've got you point. > Fourth argument is a segment handle and I use it with segment API: > > CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); > CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > These CU_ASSERTS would fail because there's no guarantee that the offset is at the start of the segment that contains it. odp_packet_seg_data(pkt, seg) returns the address of offset 0 within the specified segment, and odp_packet_seg_data_len() returns the complete length of that segment. For odp_packet_offset() the returned ptr is the address of the byte that contains the specified offset and seglen is the remaining bytes available from that point to end of packet or segment (whichever is less). > > -- > Taras Kondratiuk >
On 12/19/2014 06:54 PM, Bill Fischofer wrote: > > > On Fri, Dec 19, 2014 at 10:34 AM, Taras Kondratiuk > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> wrote: > > On 12/19/2014 05:38 PM, Bill Fischofer wrote: > > > > > > On Fri, Dec 19, 2014 at 8:46 AM, Taras Kondratiuk > > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org> > <mailto:taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org>>> wrote: > > > > > > > > > > > > + > > > + if (shift > 0) > > > + data = odp_packet_push_head(packet, shift); > > > + else > > > + data = odp_packet_pull_head(packet, -shift); > > > + > > > + CU_ASSERT(data != NULL); > > > > > > > > > Not always true. If shift is out of range NULL is the expected return. > > > > Current tests selects push/pull values to be in a valid range. So > > 'data' should not be NULL. > > > > > > Ok, however I assume at some point we'll add negative tests in which > > case this will resurface. > > Right we will add negative tests, but they won't use this function. > > > > > + CU_ASSERT(odp_packet_headroom(packet) == room - shift); > > > + CU_ASSERT(odp_packet_seg_len(packet) == seg_data_len + shift); > > > + CU_ASSERT(odp_packet_len(packet) == pkt_data_len + shift); > > > + CU_ASSERT(odp_packet_data(packet) == data); > > > > > > + CU_ASSERT(odp_packet_head(packet) == head_orig); > > > + CU_ASSERT(data == data_orig - shift); > > > > > > > > > Given the revised definition of push/pull, this test should also verify > > > that these operations do not change the various l2/l3/l4 offset values. > > > > If I'm reading specification correctly, then only > > odp_packet_push_tail() should preserve offsets: > > "Handles, pointers and offsets remain valid." > > > > > > No, the change we made is to ensure that push/pull operations on the > > head do not affect the various offsets. If the spec said nothing about > > that then the original code that adjusted them would have been correct. > > Per the revised spec, it is an error for the implementation to change > > these values as part of those operations, so the test should verify that. > > > > > > > > Other three functions (head_push/pull, tail_pull) don't promise > > anything: > > "User is responsible to update packet meta-data offsets when needed." > > > > > > This means that the implementation MUST NOT change these as part of a > > head push/pull. So tests should verify that stipulation. > > But the spec doesn't say that. I read it like: offsets are not valid > and user is responsible to update them. > I think we need clarification from Petri. > > > If the spec was silent on this point then the original implementation > which adjusted them would be conformant. If the spec said "these > offsets are indeterminate following a push/pull" then again the original > implementation would be conformant. The only reason for changing the > code was to bring it into conformance with the spec, so it is definitely > now part of the spec that these offsets MUST NOT be changed as part of > push/pull operations. So this needs to be part of the test. Your original code was kind of conformant to specification (in a way I read it), but it was doing redundant work by updating offsets which application should anyway update again. I think to add these tests the specification should clearly state that offsets must remain unchanged. Let's update the spec first and then add these tests in a follow up patch. > > > + > > > + free(data_buf); > > > +} > > > + > > > +static void packet_offset(void) > > > +{ > > > + odp_packet_t pkt = test_packet; > > > + uint32_t seg_len, full_seg_len; > > > + odp_packet_seg_t seg; > > > + uint8_t *ptr, *start_ptr; > > > + uint32_t offset; > > > + > > > + ptr = odp_packet_offset(pkt, 0, &seg_len, &seg); > > > + CU_ASSERT(seg_len > 1); > > > + CU_ASSERT(seg_len == odp_packet_seg_len(pkt)); > > > + CU_ASSERT(seg_len == > odp_packet_seg_data_len(pkt, seg)); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(ptr == odp_packet_data(pkt)); > > > + CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > > > + > > > + /* Query a second byte */ > > > + start_ptr = ptr; > > > + full_seg_len = seg_len; > > > + offset = 1; > > > + > > > + ptr = odp_packet_offset(pkt, offset, &seg_len, > NULL); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(ptr == start_ptr + offset); > > > + CU_ASSERT(seg_len == full_seg_len - offset); > > > + > > > + /* Query the last byte in a segment */ > > > + offset = full_seg_len - 1; > > > + > > > + ptr = odp_packet_offset(pkt, offset, &seg_len, > NULL); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(ptr == start_ptr + offset); > > > + CU_ASSERT(seg_len == full_seg_len - offset); > > > + > > > + /* Query the last byte in a packet */ > > > + offset = odp_packet_len(pkt) - 1; > > > + ptr = odp_packet_offset(pkt, offset, &seg_len, > NULL); > > > + CU_ASSERT(ptr != NULL); > > > + CU_ASSERT(seg_len == 1); > > > + > > > + /* Pass NULL to [out] arguments */ > > > + ptr = odp_packet_offset(pkt, 0, NULL, NULL); > > > + CU_ASSERT(ptr != NULL); > > > > > > > > > Since we have a fourth argument (not clear what the use case for > > it is, > > > but it's there) you should include a test to see that it > gets back the > > > correct (or at least a non-NULL) segment handle for the > specified > > offset > > > if it's specified. > > > > Forth argument is used to get segment length and data pointer > above. > > Will add a check for invalid segment there. > > > > > > No, the fourth argument returns the segment handle > (odp_packet_seg_t) of > > the segment that contains the requested offset. This is not a usable > > address by itself. It's not clear how an application would use > it, but > > it's part of the spec. > > I'm not sure I've got you point. > Fourth argument is a segment handle and I use it with segment API: > > CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); > CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); > > > These CU_ASSERTS would fail because there's no guarantee that the offset > is at the start of the segment that contains it. > odp_packet_seg_data(pkt, seg) returns the address of offset 0 within > the specified segment, and odp_packet_seg_data_len() returns the > complete length of that segment. For odp_packet_offset() the returned > ptr is the address of the byte that contains the specified offset and > seglen is the remaining bytes available from that point to end of packet > or segment (whichever is less). I've made following assumptions. Can you point which one is wrong? 1. We don't have a notion of an empty segment, so there should be some data in the first segment. 2. Then odp_packet_offset(pkt, 0, &seg_len, &seg) should return a first segment handle in 'seg'. It would be nice to verify this with odp_packet_first_seg() but we don't have API to compare handles yet. 3. Then these pointers should be the same: - odp_packet_data(pkt) - odp_packet_offset(pkt, 0, NULL, NULL) - odp_packet_seg_data(pkt, seg), 'seg' is the first segment 4. The same goes for lengths. 'seg_len' should be the same. - odp_packet_offset(pkt, 0, &seg_len, NULL) - seg_len = odp_packet_seg_data_len(pkt, seg), 'seg' is the first segment
diff --git a/test/validation/Makefile.am b/test/validation/Makefile.am index bbec23f..48c7fbd 100644 --- a/test/validation/Makefile.am +++ b/test/validation/Makefile.am @@ -21,6 +21,7 @@ dist_odp_shm_SOURCES = odp_shm.c common/odp_cunit_common.c dist_odp_schedule_SOURCES = odp_schedule.c common/odp_cunit_common.c dist_odp_buffer_SOURCES = buffer/odp_buffer_pool_test.c \ buffer/odp_buffer_test.c \ + buffer/odp_packet_test.c \ odp_buffer.c common/odp_cunit_common.c #For Linux generic the unimplemented crypto API functions break the diff --git a/test/validation/buffer/odp_buffer_testsuites.h b/test/validation/buffer/odp_buffer_testsuites.h index ca42c2d..715d9ac 100644 --- a/test/validation/buffer/odp_buffer_testsuites.h +++ b/test/validation/buffer/odp_buffer_testsuites.h @@ -16,10 +16,14 @@ extern CU_TestInfo buffer_pool_tests[]; extern CU_TestInfo buffer_tests[]; +extern CU_TestInfo packet_tests[]; extern int buffer_testsuite_init(void); extern int buffer_testsuite_finalize(void); +extern int packet_testsuite_init(void); +extern int packet_testsuite_finalize(void); + odp_buffer_pool_t pool_create(int buf_num, int buf_size, int buf_type); #endif /* ODP_BUFFER_TESTSUITES_H_ */ diff --git a/test/validation/buffer/odp_packet_test.c b/test/validation/buffer/odp_packet_test.c new file mode 100644 index 0000000..a664fe5 --- /dev/null +++ b/test/validation/buffer/odp_packet_test.c @@ -0,0 +1,670 @@ +/* Copyright (c) 2014, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include "odp_buffer_testsuites.h" +#include <stdlib.h> + +#define PACKET_BUF_LEN ODP_CONFIG_PACKET_BUF_LEN_MIN +/* Reserve some tailroom for tests */ +#define PACKET_TAILROOM_RESERVE 4 + +static odp_buffer_pool_t packet_pool; +static const uint32_t packet_len = PACKET_BUF_LEN - + ODP_CONFIG_PACKET_HEADROOM - + ODP_CONFIG_PACKET_TAILROOM - + PACKET_TAILROOM_RESERVE; + +odp_packet_t test_packet; + +int packet_testsuite_init(void) +{ + odp_buffer_pool_param_t params = { + .buf_size = PACKET_BUF_LEN, + .buf_align = ODP_CACHE_LINE_SIZE, + .num_bufs = 100, + .buf_type = ODP_BUFFER_TYPE_PACKET, + }; + + packet_pool = odp_buffer_pool_create("packet_pool", ODP_SHM_INVALID, + ¶ms); + if (packet_pool == ODP_BUFFER_POOL_INVALID) + return -1; + + test_packet = odp_packet_alloc(packet_pool, packet_len); + if (odp_packet_is_valid(test_packet) == 0) + return -1; + + return 0; +} + +int packet_testsuite_finalize(void) +{ + odp_packet_free(test_packet); + if (odp_buffer_pool_destroy(packet_pool) != 0) + return -1; + return 0; +} + +static void packet_alloc_free(void) +{ + odp_buffer_pool_t pool; + odp_packet_t packet; + pool = pool_create(1, PACKET_BUF_LEN, ODP_BUFFER_TYPE_PACKET); + + /* Allocate the only buffer from the pool */ + packet = odp_packet_alloc(pool, packet_len); + CU_ASSERT_FATAL(packet != ODP_PACKET_INVALID); + CU_ASSERT(odp_packet_len(packet) == packet_len); + /** @todo: is it correct to assume the pool had only one buffer? */ + CU_ASSERT_FATAL(odp_packet_alloc(pool, packet_len) == ODP_PACKET_INVALID) + + odp_packet_free(packet); + + /* Check that the buffer was returned back to the pool */ + packet = odp_packet_alloc(pool, packet_len); + CU_ASSERT_FATAL(packet != ODP_PACKET_INVALID); + CU_ASSERT(odp_packet_len(packet) == packet_len); + + odp_packet_free(packet); + CU_ASSERT(odp_buffer_pool_destroy(pool) == 0); +} + +static void packet_alloc_segmented(void) +{ + odp_packet_t pkt; + const uint32_t len = ODP_CONFIG_PACKET_BUF_LEN_MAX - + ODP_CONFIG_PACKET_HEADROOM - + ODP_CONFIG_PACKET_TAILROOM; + + pkt = odp_packet_alloc(packet_pool, len); + CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID); + CU_ASSERT(odp_packet_len(pkt) == len); + odp_packet_free(pkt); +} + +static void packet_buffer_conversion(void) +{ + odp_packet_t pkt = test_packet; + odp_packet_t tmp_pkt; + odp_buffer_t buf; + + buf = odp_packet_to_buffer(pkt); + CU_ASSERT_FATAL(buf != ODP_BUFFER_INVALID); + CU_ASSERT(odp_buffer_type(buf) == ODP_BUFFER_TYPE_PACKET); + CU_ASSERT(odp_buffer_size(buf) == odp_packet_buf_len(pkt)); + + tmp_pkt = odp_packet_from_buffer(buf); + CU_ASSERT_FATAL(tmp_pkt != ODP_PACKET_INVALID); + /** @todo: Need an API to compare packets */ +} + +static void packet_basic_metadata(void) +{ + odp_packet_t pkt = test_packet; + CU_ASSERT(odp_packet_head(pkt) != NULL); + CU_ASSERT(odp_packet_data(pkt) != NULL); + + CU_ASSERT(odp_packet_pool(pkt) != ODP_BUFFER_POOL_INVALID); + /* Packet was allocated by application so shouldn't have valid pktio. */ + CU_ASSERT(odp_packet_input(pkt) == ODP_PKTIO_INVALID); +} + +static void packet_length(void) +{ + odp_packet_t pkt = test_packet; + uint32_t buf_len, headroom, tailroom; + + buf_len = odp_packet_buf_len(pkt); + headroom = odp_packet_headroom(pkt); + tailroom = odp_packet_tailroom(pkt); + + CU_ASSERT(odp_packet_len(pkt) == packet_len); + CU_ASSERT(headroom >= ODP_CONFIG_PACKET_HEADROOM); +#if ODP_CONFIG_PACKET_TAILROOM != 0 /* Avoid 'always true' warning */ + CU_ASSERT(tailroom >= ODP_CONFIG_PACKET_TAILROOM); +#endif + CU_ASSERT(buf_len >= packet_len + headroom + tailroom); +} + +static void packet_debug(void) +{ + CU_ASSERT(odp_packet_is_valid(test_packet) == 1); + odp_packet_print(test_packet); +} + +static void packet_context(void) +{ + odp_packet_t pkt = test_packet; + char ptr_test_value = 2; + uint64_t u64_test_value = 0x0123456789abcdf; + + void *prev_ptr; + uint64_t prev_u64; + + prev_ptr = odp_packet_user_ptr(pkt); + odp_packet_user_ptr_set(pkt, &ptr_test_value); + CU_ASSERT(odp_packet_user_ptr(pkt) == &ptr_test_value); + odp_packet_user_ptr_set(pkt, prev_ptr); + + prev_u64 = odp_packet_user_u64(pkt); + odp_packet_user_u64_set(pkt, u64_test_value); + CU_ASSERT(odp_packet_user_u64(pkt) == u64_test_value); + odp_packet_user_u64_set(pkt, prev_u64); + + odp_packet_reset(pkt, packet_len); +} + +static void packet_layer_offsets(void) +{ + odp_packet_t pkt = test_packet; + uint8_t *l2_addr, *l3_addr, *l4_addr; + uint32_t seg_len; + const uint32_t l2_off = 2; + const uint32_t l3_off = l2_off + 14; + const uint32_t l4_off = l3_off + 14; + + /* Set offsets to the same value */ + odp_packet_l2_offset_set(pkt, l2_off); + odp_packet_l3_offset_set(pkt, l2_off); + odp_packet_l4_offset_set(pkt, l2_off); + + /* Addresses should be the same */ + l2_addr = odp_packet_l2_ptr(pkt, &seg_len); + CU_ASSERT(seg_len != 0); + l3_addr = odp_packet_l3_ptr(pkt, &seg_len); + CU_ASSERT(seg_len != 0); + l4_addr = odp_packet_l4_ptr(pkt, &seg_len); + CU_ASSERT(seg_len != 0); + CU_ASSERT(l2_addr != NULL); + CU_ASSERT(l2_addr == l3_addr); + CU_ASSERT(l2_addr == l4_addr); + + /* Set offsets to the different values */ + odp_packet_l2_offset_set(pkt, l2_off); + CU_ASSERT(odp_packet_l2_offset(pkt) == l2_off); + odp_packet_l3_offset_set(pkt, l3_off); + CU_ASSERT(odp_packet_l3_offset(pkt) == l3_off); + odp_packet_l4_offset_set(pkt, l4_off); + CU_ASSERT(odp_packet_l4_offset(pkt) == l4_off); + + /* Addresses should not be the same */ + l2_addr = odp_packet_l2_ptr(pkt, NULL); + CU_ASSERT(l2_addr != NULL); + l3_addr = odp_packet_l3_ptr(pkt, NULL); + CU_ASSERT(l3_addr != NULL); + l4_addr = odp_packet_l4_ptr(pkt, NULL); + CU_ASSERT(l4_addr != NULL); + + CU_ASSERT(l2_addr != l3_addr); + CU_ASSERT(l2_addr != l4_addr); + CU_ASSERT(l3_addr != l4_addr); +} + +static void _verify_headroom_shift(odp_packet_t packet, + int shift) +{ + uint32_t room = odp_packet_headroom(packet); + uint32_t seg_data_len = odp_packet_seg_len(packet); + uint32_t pkt_data_len = odp_packet_len(packet); + void *data; + char *data_orig = odp_packet_data(packet); + char *head_orig = odp_packet_head(packet); + + if (shift == 0) + return; + + if (shift > 0) + data = odp_packet_push_head(packet, shift); + else + data = odp_packet_pull_head(packet, -shift); + + CU_ASSERT(data != NULL); + CU_ASSERT(odp_packet_headroom(packet) == room - shift); + CU_ASSERT(odp_packet_seg_len(packet) == seg_data_len + shift); + CU_ASSERT(odp_packet_len(packet) == pkt_data_len + shift); + CU_ASSERT(odp_packet_data(packet) == data); + CU_ASSERT(odp_packet_head(packet) == head_orig); + CU_ASSERT(data == data_orig - shift); +} + +static void packet_headroom(void) +{ + odp_packet_t pkt = test_packet; + uint32_t room; + uint32_t seg_data_len; + uint32_t push_val, pull_val; + + room = odp_packet_headroom(pkt); + CU_ASSERT(room >= ODP_CONFIG_PACKET_HEADROOM); + seg_data_len = odp_packet_seg_len(pkt); + CU_ASSERT(seg_data_len >= 1); + /** @todo: should be len - 1 */ + pull_val = seg_data_len / 2; + push_val = room; + + _verify_headroom_shift(pkt, -pull_val); + _verify_headroom_shift(pkt, push_val + pull_val); + _verify_headroom_shift(pkt, -push_val); +} + +static void _verify_tailroom_shift(odp_packet_t pkt, + int shift) +{ + odp_packet_seg_t seg; + uint32_t room; + uint32_t seg_data_len, pkt_data_len; + void *tail; + char *tail_orig; + + if (shift == 0) + return; + + room = odp_packet_tailroom(pkt); + pkt_data_len = odp_packet_len(pkt); + tail_orig = odp_packet_tail(pkt); + + seg = odp_packet_last_seg(pkt); + CU_ASSERT(seg != ODP_SEGMENT_INVALID); + seg_data_len = odp_packet_seg_data_len(pkt, seg); + + if (shift > 0) + tail = odp_packet_push_tail(pkt, shift); + else + tail = odp_packet_pull_tail(pkt, -shift); + + CU_ASSERT(tail != NULL); + CU_ASSERT(odp_packet_seg_data_len(pkt, seg) == seg_data_len + shift); + CU_ASSERT(odp_packet_len(pkt) == pkt_data_len + shift); + CU_ASSERT(odp_packet_tailroom(pkt) == room - shift); + if (room == 0 || (room - shift) == 0) + return; + if (shift > 0) { + CU_ASSERT(odp_packet_tail(pkt) == tail_orig + shift); + CU_ASSERT(tail == tail_orig); + } else { + CU_ASSERT(odp_packet_tail(pkt) == tail); + CU_ASSERT(tail == tail_orig + shift); + } +} + +static void packet_tailroom(void) +{ + odp_packet_t pkt = test_packet; + odp_packet_seg_t segment; + uint32_t room; + uint32_t seg_data_len; + uint32_t push_val, pull_val; + + segment = odp_packet_last_seg(pkt); + CU_ASSERT(segment != ODP_SEGMENT_INVALID); + room = odp_packet_tailroom(pkt); +#if ODP_CONFIG_PACKET_TAILROOM != 0 /* Avoid 'always true' warning */ + CU_ASSERT(room >= ODP_CONFIG_PACKET_TAILROOM); +#endif + seg_data_len = odp_packet_seg_data_len(pkt, segment); + CU_ASSERT(seg_data_len >= 1); + /** @todo: should be len - 1 */ + pull_val = seg_data_len / 2; + /* Leave one byte in a tailroom for odp_packet_tail() to succeed */ + push_val = (room > 0) ? room - 1 : room; + + _verify_tailroom_shift(pkt, -pull_val); + _verify_tailroom_shift(pkt, push_val + pull_val); + _verify_tailroom_shift(pkt, -push_val); +} + +static void packet_segments(void) +{ + int num_segs, seg_index; + uint32_t data_len, buf_len; + odp_packet_seg_t seg; + odp_packet_t pkt = test_packet; + + CU_ASSERT(odp_packet_is_valid(pkt) == 1); + + num_segs = odp_packet_num_segs(pkt); + CU_ASSERT(num_segs != 0); + + if (!odp_packet_is_segmented(pkt)) + CU_ASSERT(num_segs == 1); + + seg = odp_packet_first_seg(pkt); + buf_len = 0; + data_len = 0; + seg_index = 0; + while (seg_index < num_segs && seg != ODP_PACKET_SEG_INVALID) { + uint32_t seg_data_len, seg_buf_len; + void *seg_buf_addr, *seg_data; + + seg_buf_addr = odp_packet_seg_buf_addr(pkt, seg); + seg_buf_len = odp_packet_seg_buf_len(pkt, seg); + seg_data_len = odp_packet_seg_data_len(pkt, seg); + seg_data = odp_packet_seg_data(pkt, seg); + + CU_ASSERT(seg_buf_len > 0); + CU_ASSERT(seg_data_len > 0); + CU_ASSERT(seg_buf_len > seg_data_len); + CU_ASSERT(seg_data != NULL); + CU_ASSERT(seg_buf_addr != NULL); + CU_ASSERT(seg_data > seg_buf_addr); + + buf_len += seg_buf_len; + data_len += seg_data_len; + + /** @todo: touch memory in a segment */ + seg_index++; + seg = odp_packet_next_seg(pkt, seg); + } + + CU_ASSERT(seg_index == num_segs); + CU_ASSERT(buf_len == odp_buffer_size(odp_packet_to_buffer(pkt))); + CU_ASSERT(data_len == odp_packet_len(pkt)); + + if (seg_index == num_segs) + CU_ASSERT(seg == ODP_PACKET_SEG_INVALID); +} + +static void packet_segment_last(void) +{ + odp_packet_t pkt = test_packet; + odp_packet_seg_t seg; + + seg = odp_packet_last_seg(pkt); + CU_ASSERT_FATAL(seg != ODP_PACKET_SEG_INVALID); + + seg = odp_packet_next_seg(pkt, seg); + CU_ASSERT(seg == ODP_PACKET_SEG_INVALID); +} + +#define TEST_INFLAG(packet, flag) \ +do { \ + odp_packet_has_##flag##_set(packet, 0); \ + CU_ASSERT(odp_packet_has_##flag(packet) == 0); \ + odp_packet_has_##flag##_set(packet, 1); \ + CU_ASSERT(odp_packet_has_##flag(packet) == 1); \ +} while (0) + +static void packet_in_flags(void) +{ + odp_packet_t pkt = test_packet; + + TEST_INFLAG(pkt, l2); + TEST_INFLAG(pkt, l3); + TEST_INFLAG(pkt, l4); + TEST_INFLAG(pkt, eth); + TEST_INFLAG(pkt, jumbo); + TEST_INFLAG(pkt, vlan); + TEST_INFLAG(pkt, vlan_qinq); + TEST_INFLAG(pkt, arp); + TEST_INFLAG(pkt, ipv4); + TEST_INFLAG(pkt, ipv6); + TEST_INFLAG(pkt, ipfrag); + TEST_INFLAG(pkt, ipopt); + TEST_INFLAG(pkt, ipsec); + TEST_INFLAG(pkt, udp); + TEST_INFLAG(pkt, tcp); + TEST_INFLAG(pkt, sctp); + TEST_INFLAG(pkt, icmp); +} + +static void packet_error_flags(void) +{ + odp_packet_t pkt = test_packet; + int err; + + /** + * The packet have not been classified so it doesn't have error flag + * properly set. Just check that function return one of allowed values. + * @todo: check classified packet when classifier is added in place. + */ + err = odp_packet_error(pkt); + CU_ASSERT(err == 0 || err == 1); + + err = odp_packet_errflag_frame_len(pkt); + CU_ASSERT(err == 0 || err == 1); +} + +static void packet_out_flags(void) +{ + odp_packet_override_l4_chksum(test_packet); + CU_PASS("Current API doesn't return any error code\n"); +} + +static void packet_add_rem_data(void) +{ + odp_packet_t pkt, new_pkt; + uint32_t pkt_len, offset, add_len; + + pkt = odp_packet_alloc(packet_pool, PACKET_BUF_LEN); + CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID); + + pkt_len = odp_packet_len(pkt); + /* Insert one more packet length in the middle of a packet */ + offset = pkt_len / 2; + add_len = pkt_len; + + new_pkt = odp_packet_add_data(pkt, offset, add_len); + CU_ASSERT(new_pkt != ODP_PACKET_INVALID); + if (new_pkt == ODP_PACKET_INVALID) + goto free_packet; + CU_ASSERT(odp_packet_len(new_pkt) == pkt_len + add_len); + pkt = new_pkt; + + pkt_len = odp_packet_len(pkt); + new_pkt = odp_packet_rem_data(pkt, offset, add_len); + CU_ASSERT(new_pkt != ODP_PACKET_INVALID); + if (new_pkt == ODP_PACKET_INVALID) + goto free_packet; + CU_ASSERT(odp_packet_len(new_pkt) == pkt_len - add_len); + pkt = new_pkt; + +free_packet: + odp_packet_free(pkt); +} + + +#define COMPARE_INFLAG(p1, p2, flag) \ + CU_ASSERT(odp_packet_has_##flag(p1) == odp_packet_has_##flag(p2)) + +static void _packet_compare_inflags(odp_packet_t pkt1, odp_packet_t pkt2) +{ + COMPARE_INFLAG(pkt1, pkt2, l2); + COMPARE_INFLAG(pkt1, pkt2, l3); + COMPARE_INFLAG(pkt1, pkt2, l4); + COMPARE_INFLAG(pkt1, pkt2, eth); + COMPARE_INFLAG(pkt1, pkt2, jumbo); + COMPARE_INFLAG(pkt1, pkt2, eth); + COMPARE_INFLAG(pkt1, pkt2, vlan); + COMPARE_INFLAG(pkt1, pkt2, vlan_qinq); + COMPARE_INFLAG(pkt1, pkt2, arp); + COMPARE_INFLAG(pkt1, pkt2, ipv4); + COMPARE_INFLAG(pkt1, pkt2, ipv6); + COMPARE_INFLAG(pkt1, pkt2, ipfrag); + COMPARE_INFLAG(pkt1, pkt2, ipopt); + COMPARE_INFLAG(pkt1, pkt2, ipsec); + COMPARE_INFLAG(pkt1, pkt2, udp); + COMPARE_INFLAG(pkt1, pkt2, tcp); + COMPARE_INFLAG(pkt1, pkt2, sctp); + COMPARE_INFLAG(pkt1, pkt2, icmp); +} + +struct pktcmp_ctx { + odp_packet_t pkt; + odp_packet_seg_t seg; + char *ptr; + uint32_t len; +}; + +static void _packet_compare_data(odp_packet_t pkt1, odp_packet_t pkt2) +{ + const int pkt_num = 2; + int i; + struct pktcmp_ctx info[pkt_num]; + uint32_t pkt_len = odp_packet_len(pkt1); + + CU_ASSERT_FATAL(pkt_len == odp_packet_len(pkt2)); + + info[0].pkt = pkt1; + info[1].pkt = pkt2; + + for (i = 0; i < pkt_num; i++) { + info[i].seg = odp_packet_first_seg(info[i].pkt); + CU_ASSERT_FATAL(info[i].seg != ODP_PACKET_SEG_INVALID); + + info[i].len = odp_packet_seg_data_len(info[i].pkt, info[i].seg); + info[i].ptr = odp_packet_seg_data(info[i].pkt, info[i].seg); + CU_ASSERT_FATAL(info[i].len > 0); + CU_ASSERT_FATAL(info[i].ptr != NULL); + } + + while (pkt_len > 0) { + uint32_t min_seg_len = (info[0].len < info[1].len) ? + info[0].len : info[1].len; + + CU_ASSERT_FATAL(!memcmp(info[0].ptr, info[1].ptr, min_seg_len)); + + if (pkt_len <= min_seg_len) + return; + + pkt_len -= min_seg_len; + for (i = 0; i < pkt_num; i++) { + info[i].len -= min_seg_len; + if (info[i].len != 0) { + info[i].ptr += min_seg_len; + continue; + } + + info[i].seg = odp_packet_next_seg(info[i].pkt, + info[i].seg); + CU_ASSERT_FATAL(info[i].seg != ODP_PACKET_SEG_INVALID); + + info[i].len = odp_packet_seg_data_len(info[i].pkt, + info[i].seg); + info[i].ptr = odp_packet_seg_data(info[i].pkt, + info[i].seg); + CU_ASSERT_FATAL(info[i].len > 0); + CU_ASSERT_FATAL(info[i].ptr != NULL); + } + } +} + +static void packet_copy(void) +{ + odp_packet_t pkt = test_packet; + odp_packet_t pkt_copy; + odp_buffer_pool_t pool; + + /** @todo: fill original packet with some data */ + pool = odp_packet_pool(pkt); + CU_ASSERT_FATAL(pool != ODP_BUFFER_POOL_INVALID); + pkt_copy = odp_packet_copy(pkt, odp_packet_pool(pkt)); + CU_ASSERT_FATAL(pkt_copy != ODP_PACKET_INVALID); + + CU_ASSERT(odp_packet_len(pkt) == odp_packet_len(pkt_copy)); + + _packet_compare_inflags(pkt, pkt_copy); + _packet_compare_data(pkt, pkt_copy); + odp_packet_free(pkt_copy); +} + +static void packet_copydata(void) +{ + odp_packet_t pkt = test_packet; + uint32_t pkt_len = odp_packet_len(pkt); + uint8_t *data_buf; + uint32_t i; + int correct_memory; + + CU_ASSERT_FATAL(pkt_len > 0); + + data_buf = malloc(pkt_len); + CU_ASSERT_FATAL(data_buf != NULL); + + for (i = 0; i < pkt_len; i++) + data_buf[i] = (uint8_t)i; + + CU_ASSERT(!odp_packet_copydata_in(pkt, 0, pkt_len, data_buf)); + memset(data_buf, 0, pkt_len); + CU_ASSERT(!odp_packet_copydata_out(pkt, 0, pkt_len, data_buf)); + + correct_memory = 1; + for (i = 0; i < pkt_len; i++) + if (data_buf[i] != (uint8_t)i) { + correct_memory = 0; + break; + } + CU_ASSERT(correct_memory); + + free(data_buf); +} + +static void packet_offset(void) +{ + odp_packet_t pkt = test_packet; + uint32_t seg_len, full_seg_len; + odp_packet_seg_t seg; + uint8_t *ptr, *start_ptr; + uint32_t offset; + + ptr = odp_packet_offset(pkt, 0, &seg_len, &seg); + CU_ASSERT(seg_len > 1); + CU_ASSERT(seg_len == odp_packet_seg_len(pkt)); + CU_ASSERT(seg_len == odp_packet_seg_data_len(pkt, seg)); + CU_ASSERT(ptr != NULL); + CU_ASSERT(ptr == odp_packet_data(pkt)); + CU_ASSERT(ptr == odp_packet_seg_data(pkt, seg)); + + /* Query a second byte */ + start_ptr = ptr; + full_seg_len = seg_len; + offset = 1; + + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); + CU_ASSERT(ptr != NULL); + CU_ASSERT(ptr == start_ptr + offset); + CU_ASSERT(seg_len == full_seg_len - offset); + + /* Query the last byte in a segment */ + offset = full_seg_len - 1; + + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); + CU_ASSERT(ptr != NULL); + CU_ASSERT(ptr == start_ptr + offset); + CU_ASSERT(seg_len == full_seg_len - offset); + + /* Query the last byte in a packet */ + offset = odp_packet_len(pkt) - 1; + ptr = odp_packet_offset(pkt, offset, &seg_len, NULL); + CU_ASSERT(ptr != NULL); + CU_ASSERT(seg_len == 1); + + /* Pass NULL to [out] arguments */ + ptr = odp_packet_offset(pkt, 0, NULL, NULL); + CU_ASSERT(ptr != NULL); +} + +CU_TestInfo packet_tests[] = { + _CU_TEST_INFO(packet_alloc_free), + _CU_TEST_INFO(packet_alloc_segmented), + _CU_TEST_INFO(packet_basic_metadata), + _CU_TEST_INFO(packet_debug), + _CU_TEST_INFO(packet_length), + _CU_TEST_INFO(packet_headroom), + _CU_TEST_INFO(packet_tailroom), + _CU_TEST_INFO(packet_context), + _CU_TEST_INFO(packet_buffer_conversion), + _CU_TEST_INFO(packet_layer_offsets), + _CU_TEST_INFO(packet_segments), + _CU_TEST_INFO(packet_segment_last), + _CU_TEST_INFO(packet_in_flags), + _CU_TEST_INFO(packet_out_flags), + _CU_TEST_INFO(packet_error_flags), + _CU_TEST_INFO(packet_add_rem_data), + _CU_TEST_INFO(packet_copy), + _CU_TEST_INFO(packet_copydata), + _CU_TEST_INFO(packet_offset), + CU_TEST_INFO_NULL, +}; diff --git a/test/validation/odp_buffer.c b/test/validation/odp_buffer.c index 8aa61a9..1102780 100644 --- a/test/validation/odp_buffer.c +++ b/test/validation/odp_buffer.c @@ -15,5 +15,10 @@ CU_SuiteInfo odp_testsuites[] = { .pInitFunc = buffer_testsuite_init, .pCleanupFunc = buffer_testsuite_finalize, }, + { .pName = "packet tests", + .pTests = packet_tests, + .pInitFunc = packet_testsuite_init, + .pCleanupFunc = packet_testsuite_finalize, + }, CU_SUITE_INFO_NULL, };
Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> --- test/validation/Makefile.am | 1 + test/validation/buffer/odp_buffer_testsuites.h | 4 + test/validation/buffer/odp_packet_test.c | 670 +++++++++++++++++++++++++ test/validation/odp_buffer.c | 5 + 4 files changed, 680 insertions(+) create mode 100644 test/validation/buffer/odp_packet_test.c