Hi Ivan, Agreed. Splitting can be done. Do you have any other comments apart from splitting?
Regards, Bala On 20 October 2015 at 15:52, Ivan Khoronzhuk <[email protected]> wrote: > Hi, Bala > > Propose to split this on 4 patches for clarity and history reason: > > 1 - destroy_inq() move in order to use as common function > 2 - use TCP header length instead of hardcode > 3 - remove unneeded odp_pool_lookup() and rename pool on pool_default > 4 - align seq_num verification > > > On 20.10.15 09:48, Balasubramanian Manoharan wrote: >> >> The following minor code modifications are implemented in this patch >> * Move destoy_inq() function to common file >> * TCP header length calculated from packet >> * Redundant pool lookup function removed >> >> Signed-off-by: Balasubramanian Manoharan <[email protected]> >> --- >> .../classification/odp_classification_common.c | 41 >> +++++++++++++++++-- >> .../classification/odp_classification_test_pmr.c | 46 >> +++------------------- >> .../classification/odp_classification_tests.c | 31 ++------------- >> .../classification/odp_classification_testsuites.h | 2 +- >> 4 files changed, 47 insertions(+), 73 deletions(-) >> >> diff --git a/test/validation/classification/odp_classification_common.c >> b/test/validation/classification/odp_classification_common.c >> index b975dfb..ba3ade4 100644 >> --- a/test/validation/classification/odp_classification_common.c >> +++ b/test/validation/classification/odp_classification_common.c >> @@ -17,12 +17,40 @@ typedef struct cls_test_packet { >> uint32be_t seq; >> } cls_test_packet_t; >> >> +int destroy_inq(odp_pktio_t pktio) >> +{ >> + odp_queue_t inq; >> + odp_event_t ev; >> + >> + inq = odp_pktio_inq_getdef(pktio); >> + >> + if (inq == ODP_QUEUE_INVALID) { >> + CU_FAIL("attempting to destroy invalid inq"); >> + return -1; >> + } >> + >> + if (0 > odp_pktio_inq_remdef(pktio)) >> + return -1; >> + >> + while (1) { >> + ev = odp_schedule(NULL, ODP_SCHED_NO_WAIT); >> + >> + if (ev != ODP_EVENT_INVALID) >> + odp_event_free(ev); >> + else >> + break; >> + } >> + >> + return odp_queue_destroy(inq); >> +} >> + >> int cls_pkt_set_seq(odp_packet_t pkt) >> { >> static uint32_t seq; >> cls_test_packet_t data; >> uint32_t offset; >> odph_ipv4hdr_t *ip; >> + odph_tcphdr_t *tcp; >> int status; >> >> data.magic = DATA_MAGIC; >> @@ -35,9 +63,11 @@ int cls_pkt_set_seq(odp_packet_t pkt) >> if (ip->proto == ODPH_IPPROTO_UDP) >> status = odp_packet_copydata_in(pkt, offset + >> ODPH_UDPHDR_LEN, >> sizeof(data), &data); >> - else >> - status = odp_packet_copydata_in(pkt, offset + >> ODPH_TCPHDR_LEN, >> + else { >> + tcp = (odph_tcphdr_t *)odp_packet_l4_ptr(pkt, NULL); >> + status = odp_packet_copydata_in(pkt, offset + tcp->hl * 4, >> sizeof(data), &data); >> + } >> >> return status; >> } >> @@ -47,6 +77,7 @@ uint32_t cls_pkt_get_seq(odp_packet_t pkt) >> uint32_t offset; >> cls_test_packet_t data; >> odph_ipv4hdr_t *ip; >> + odph_tcphdr_t *tcp; >> >> ip = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, NULL); >> offset = odp_packet_l4_offset(pkt); >> @@ -57,9 +88,11 @@ uint32_t cls_pkt_get_seq(odp_packet_t pkt) >> if (ip->proto == ODPH_IPPROTO_UDP) >> odp_packet_copydata_out(pkt, offset + ODPH_UDPHDR_LEN, >> sizeof(data), &data); >> - else >> - odp_packet_copydata_out(pkt, offset + ODPH_TCPHDR_LEN, >> + else { >> + tcp = (odph_tcphdr_t *)odp_packet_l4_ptr(pkt, NULL); >> + odp_packet_copydata_out(pkt, offset + tcp->hl * 4, >> sizeof(data), &data); >> + } >> >> if (data.magic == DATA_MAGIC) >> return data.seq; >> diff --git a/test/validation/classification/odp_classification_test_pmr.c >> b/test/validation/classification/odp_classification_test_pmr.c >> index e794bda..4bfe0cb 100644 >> --- a/test/validation/classification/odp_classification_test_pmr.c >> +++ b/test/validation/classification/odp_classification_test_pmr.c >> @@ -17,36 +17,8 @@ static odp_pool_t pool_default; >> /** sequence number of IP packets */ >> odp_atomic_u32_t seq; >> >> -static int destroy_inq(odp_pktio_t pktio) >> -{ >> - odp_queue_t inq; >> - odp_event_t ev; >> - >> - inq = odp_pktio_inq_getdef(pktio); >> - >> - if (inq == ODP_QUEUE_INVALID) { >> - CU_FAIL("attempting to destroy invalid inq"); >> - return -1; >> - } >> - >> - if (0 > odp_pktio_inq_remdef(pktio)) >> - return -1; >> - >> - while (1) { >> - ev = odp_schedule(NULL, ODP_SCHED_NO_WAIT); >> - >> - if (ev != ODP_EVENT_INVALID) >> - odp_buffer_free(odp_buffer_from_event(ev)); >> - else >> - break; >> - } >> - >> - return odp_queue_destroy(inq); >> -} >> - >> int classification_suite_pmr_init(void) >> { >> - odp_pool_t pool; >> odp_pool_param_t param; >> >> odp_pool_param_init(¶m); >> @@ -55,16 +27,12 @@ int classification_suite_pmr_init(void) >> param.pkt.num = SHM_PKT_NUM_BUFS; >> param.type = ODP_POOL_PACKET; >> >> - pool = odp_pool_create("classification_pmr_pool", ¶m); >> - if (ODP_POOL_INVALID == pool) { >> + pool_default = odp_pool_create("classification_pmr_pool", ¶m); >> + if (ODP_POOL_INVALID == pool_default) { >> fprintf(stderr, "Packet pool creation failed.\n"); >> return -1; >> } >> >> - pool_default = odp_pool_lookup("classification_pmr_pool"); >> - if (pool_default == ODP_POOL_INVALID) >> - return -1; >> - >> odp_atomic_init_u32(&seq, 0); >> return 0; >> } >> @@ -73,11 +41,9 @@ odp_pktio_t create_pktio(odp_queue_type_t q_type) >> { >> odp_pktio_t pktio; >> odp_pktio_param_t pktio_param; >> - odp_pool_t pool; >> int ret; >> >> - pool = odp_pool_lookup("classification_pmr_pool"); >> - if (pool == ODP_POOL_INVALID) >> + if (pool_default == ODP_POOL_INVALID) >> return ODP_PKTIO_INVALID; >> >> odp_pktio_param_init(&pktio_param); >> @@ -86,9 +52,9 @@ odp_pktio_t create_pktio(odp_queue_type_t q_type) >> else >> pktio_param.in_mode = ODP_PKTIN_MODE_SCHED; >> >> - pktio = odp_pktio_open("loop", pool, &pktio_param); >> + pktio = odp_pktio_open("loop", pool_default, &pktio_param); >> if (pktio == ODP_PKTIO_INVALID) { >> - ret = odp_pool_destroy(pool); >> + ret = odp_pool_destroy(pool_default); >> if (ret) >> fprintf(stderr, "unable to destroy pool.\n"); >> return ODP_PKTIO_INVALID; >> @@ -477,7 +443,6 @@ static void >> classification_test_pmr_term_udp_sport(void) >> >> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >> CU_ASSERT(pkt != ODP_PACKET_INVALID); >> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >> CU_ASSERT(retqueue == defqueue); >> odp_packet_free(pkt); >> @@ -557,7 +522,6 @@ static void classification_test_pmr_term_ipproto(void) >> >> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >> CU_ASSERT(pkt != ODP_PACKET_INVALID); >> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >> CU_ASSERT(retqueue == defqueue); >> >> diff --git a/test/validation/classification/odp_classification_tests.c >> b/test/validation/classification/odp_classification_tests.c >> index fe55419..62e67d1 100644 >> --- a/test/validation/classification/odp_classification_tests.c >> +++ b/test/validation/classification/odp_classification_tests.c >> @@ -22,33 +22,6 @@ static odp_pktio_t pktio_loop; >> /** sequence number of IP packets */ >> odp_atomic_u32_t seq; >> >> -static int destroy_inq(odp_pktio_t pktio) >> -{ >> - odp_queue_t inq; >> - odp_event_t ev; >> - >> - inq = odp_pktio_inq_getdef(pktio); >> - >> - if (inq == ODP_QUEUE_INVALID) { >> - CU_FAIL("attempting to destroy invalid inq"); >> - return -1; >> - } >> - >> - if (0 > odp_pktio_inq_remdef(pktio)) >> - return -1; >> - >> - while (1) { >> - ev = odp_schedule(NULL, ODP_SCHED_NO_WAIT); >> - >> - if (ev != ODP_EVENT_INVALID) >> - odp_event_free(ev); >> - else >> - break; >> - } >> - >> - return odp_queue_destroy(inq); >> -} >> - >> int classification_suite_init(void) >> { >> odp_pool_param_t param; >> @@ -358,9 +331,12 @@ void test_pktio_error_cos(void) >> { >> odp_queue_t queue; >> odp_packet_t pkt; >> + uint32_t seqno = 0; >> >> /*Create an error packet */ >> pkt = create_packet(pool_default, false, &seq, true); >> + seqno = cls_pkt_get_seq(pkt); >> + CU_ASSERT(seqno != TEST_SEQ_INVALID); >> odph_ipv4hdr_t *ip = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, >> NULL); >> >> /* Incorrect IpV4 version */ >> @@ -370,6 +346,7 @@ void test_pktio_error_cos(void) >> >> pkt = receive_packet(&queue, ODP_TIME_SEC); >> CU_ASSERT(pkt != ODP_PACKET_INVALID); >> + CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >> /* Error packet should be received in error queue */ >> CU_ASSERT(queue == queue_list[CLS_ERROR]); >> odp_packet_free(pkt); >> diff --git >> a/test/validation/classification/odp_classification_testsuites.h >> b/test/validation/classification/odp_classification_testsuites.h >> index 33547a7..a7a8baa 100644 >> --- a/test/validation/classification/odp_classification_testsuites.h >> +++ b/test/validation/classification/odp_classification_testsuites.h >> @@ -43,6 +43,6 @@ void configure_pmr_cos(void); >> void test_pmr_cos(void); >> void configure_pktio_pmr_match_set_cos(void); >> void test_pktio_pmr_match_set_cos(void); >> - >> +int destroy_inq(odp_pktio_t pktio); >> >> #endif /* ODP_BUFFER_TESTSUITES_H_ */ >> > > -- > Regards, > Ivan Khoronzhuk _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
