| Message ID | 1418989601-14300-5-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
No functional issues, just style and consistency comments. On 19 December 2014 at 06:46, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > test/validation/odp_pktio.c | 89 > ++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 80 insertions(+), 9 deletions(-) > > diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c > index f7dc80b..4e3cc00 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -403,6 +403,74 @@ static void test_odp_pktio_sched_multi(void) > pktio_test_txrx(ODP_QUEUE_TYPE_SCHED, 4); > } > > +static void pktio_test_mtu(void) > +{ > + int ret; > + int mtu; > + odp_pktio_t pktio = create_pktio(iface_name[0]); > + > + mtu = odp_pktio_mtu(pktio); > + CU_ASSERT(mtu > 0); > + > + printf(" %d ", mtu); > Does this print help the test ? or was it just used during debugging ? If it adds value to the user of the test thats fine > + > + ret = odp_pktio_close(pktio); > + CU_ASSERT(ret == 0); > + > + return; > +} > + > +static void pktio_test_promisc(void) > +{ > + int ret; > + odp_pktio_t pktio = create_pktio(iface_name[0]); > + > + ret = odp_pktio_promisc_mode_set(pktio, 1); > + CU_ASSERT(0 == ret); > + > + /* Check */ > Comment adds no value - checks what ? if you make a comment out of it I think it should be a complete sentence. + ret = odp_pktio_promisc_mode(pktio); > + CU_ASSERT(1 == ret); > + > + ret = odp_pktio_promisc_mode_set(pktio, 0); > + CU_ASSERT(0 == ret); > + > + /* Check */ > Comment adds no value > + ret = odp_pktio_promisc_mode(pktio); > + CU_ASSERT(0 == ret); > + > + ret = odp_pktio_close(pktio); > + CU_ASSERT(ret == 0); > Consistency in order of arranging the constant and the variable in the argument 0 == ret vs ret == 0 - should all match within one file. > + > + return; > +} > + > +static void pktio_test_mac(void) > +{ > + unsigned char mac_addr[ODPH_ETHADDR_LEN]; > + size_t mac_len; > + int ret; > + odp_pktio_t pktio = create_pktio(iface_name[0]); > + > + printf("testing mac for %s\n", iface_name[0]); > + > + mac_len = odp_pktio_mac_addr(pktio, mac_addr, ODPH_ETHADDR_LEN); > + CU_ASSERT(ODPH_ETHADDR_LEN == mac_len); > + > + printf(" %X:%X:%X:%X:%X:%X ", > + mac_addr[0], mac_addr[1], mac_addr[2], > + mac_addr[3], mac_addr[4], mac_addr[5]); > + > Does this print help the test, or was it for debugging the test ? > + /* Fail case */ > As a comment could this be more complete as a sentence to a human ? > + mac_len = odp_pktio_mac_addr(pktio, mac_addr, 2); > + CU_ASSERT(0 == mac_len); > + > + ret = odp_pktio_close(pktio); > + CU_ASSERT(ret == 0); > + > + return; > +} > + > static void test_odp_pktio_open(void) > { > odp_pktio_t pktio; > @@ -483,19 +551,22 @@ static int term_pktio_suite(void) > } > > CU_TestInfo pktio_tests[] = { > - {"pktio open", test_odp_pktio_open}, > - {"pktio close", test_odp_pktio_close}, > - {"pktio inq", test_odp_pktio_inq}, > - {"pktio outq", test_odp_pktio_outq}, > - {"pktio poll queues", test_odp_pktio_poll_queue}, > - {"pktio poll multi", test_odp_pktio_poll_multi}, > - {"pktio sched queues", test_odp_pktio_sched_queue}, > - {"pktio sched multi", test_odp_pktio_sched_multi}, > + {"pktio open", test_odp_pktio_open}, > + {"pktio close", test_odp_pktio_close}, > + {"pktio inq", test_odp_pktio_inq}, > + {"pktio outq", test_odp_pktio_outq}, > + {"pktio poll queues", test_odp_pktio_poll_queue}, > + {"pktio poll multi", test_odp_pktio_poll_multi}, > + {"pktio sched queues", test_odp_pktio_sched_queue}, > + {"pktio sched multi", test_odp_pktio_sched_multi}, > + {"pktio mtu", pktio_test_mtu}, > This does not follow the existing naming style in this file, it should be test_odp_pktio_test_mtu > + {"pktio promisc mode", pktio_test_promisc}, > This does not follow the existing naming style in this file > + {"pktio mac", pktio_test_mac}, > This does not follow the existing naming style in this file > CU_TEST_INFO_NULL > }; > Nit Was it better to convert all the spaces to tabs? It looks like you could have added your extra prints without converting them all to tabs and thus reduced the size of the diff. > > CU_SuiteInfo odp_testsuites[] = { > - {"odp_pktio", > + {"Packet I/O", > init_pktio_suite, term_pktio_suite, NULL, NULL, > pktio_tests}, > CU_SUITE_INFO_NULL > }; > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 12/22/2014 04:56 PM, Mike Holmes wrote: > No functional issues, just style and consistency comments. > > On 19 December 2014 at 06:46, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > test/validation/odp_pktio.c | 89 > ++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 80 insertions(+), 9 deletions(-) > > diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c > index f7dc80b..4e3cc00 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -403,6 +403,74 @@ static void test_odp_pktio_sched_multi(void) > pktio_test_txrx(ODP_QUEUE_TYPE_SCHED, 4); > } > > +static void pktio_test_mtu(void) > +{ > + int ret; > + int mtu; > + odp_pktio_t pktio = create_pktio(iface_name[0]); > + > + mtu = odp_pktio_mtu(pktio); > + CU_ASSERT(mtu > 0); > + > + printf(" %d ", mtu); > > > Does this print help the test ? or was it just used during debugging ? > If it adds value to the user of the test thats fine > > + > + ret = odp_pktio_close(pktio); > + CU_ASSERT(ret == 0); > + > + return; > +} > + > +static void pktio_test_promisc(void) > +{ > + int ret; > + odp_pktio_t pktio = create_pktio(iface_name[0]); > + > + ret = odp_pktio_promisc_mode_set(pktio, 1); > + CU_ASSERT(0 == ret); > + > + /* Check */ > > > Comment adds no value - checks what ? if you make a comment out of it > I think it should be a complete sentence. Check meas, we first set up value. Then check that it was set. > > + ret = odp_pktio_promisc_mode(pktio); > + CU_ASSERT(1 == ret); > + > + ret = odp_pktio_promisc_mode_set(pktio, 0); > + CU_ASSERT(0 == ret); > + > + /* Check */ > > > Comment adds no value Also here, set to 0 and check. > + ret = odp_pktio_promisc_mode(pktio); > + CU_ASSERT(0 == ret); > + > + ret = odp_pktio_close(pktio); > + CU_ASSERT(ret == 0); > > > Consistency in order of arranging the constant and the variable in the > argument 0 == ret vs ret == 0 - should all match within one file. ok. > > + > + return; > +} > + > +static void pktio_test_mac(void) > +{ > + unsigned char mac_addr[ODPH_ETHADDR_LEN]; > + size_t mac_len; > + int ret; > + odp_pktio_t pktio = create_pktio(iface_name[0]); > + > + printf("testing mac for %s\n", iface_name[0]); > + > + mac_len = odp_pktio_mac_addr(pktio, mac_addr, > ODPH_ETHADDR_LEN); > + CU_ASSERT(ODPH_ETHADDR_LEN == mac_len); > + > + printf(" %X:%X:%X:%X:%X:%X ", > + mac_addr[0], mac_addr[1], mac_addr[2], > + mac_addr[3], mac_addr[4], mac_addr[5]); > + > > > Does this print help the test, or was it for debugging the test ? It's for more info. So you can see on which interface was the test. And if you know original value than you can see that it's expected. > + /* Fail case */ > > > As a comment could this be more complete as a sentence to a human ? ok. > > + mac_len = odp_pktio_mac_addr(pktio, mac_addr, 2); > + CU_ASSERT(0 == mac_len); > + > + ret = odp_pktio_close(pktio); > + CU_ASSERT(ret == 0); > + > + return; > +} > + > static void test_odp_pktio_open(void) > { > odp_pktio_t pktio; > @@ -483,19 +551,22 @@ static int term_pktio_suite(void) > } > > CU_TestInfo pktio_tests[] = { > - {"pktio open", test_odp_pktio_open}, > - {"pktio close", test_odp_pktio_close}, > - {"pktio inq", test_odp_pktio_inq}, > - {"pktio outq", test_odp_pktio_outq}, > - {"pktio poll queues", test_odp_pktio_poll_queue}, > - {"pktio poll multi", test_odp_pktio_poll_multi}, > - {"pktio sched queues", test_odp_pktio_sched_queue}, > - {"pktio sched multi", test_odp_pktio_sched_multi}, > + {"pktio open", test_odp_pktio_open}, > + {"pktio close", test_odp_pktio_close}, > + {"pktio inq", test_odp_pktio_inq}, > + {"pktio outq", test_odp_pktio_outq}, > + {"pktio poll queues", test_odp_pktio_poll_queue}, > + {"pktio poll multi", test_odp_pktio_poll_multi}, > + {"pktio sched queues", test_odp_pktio_sched_queue}, > + {"pktio sched multi", test_odp_pktio_sched_multi}, > + {"pktio mtu", pktio_test_mtu}, > > > This does not follow the existing naming style in this file, it should > be test_odp_pktio_test_mtu ok. > > + {"pktio promisc mode", pktio_test_promisc}, > > > This does not follow the existing naming style in this file ok. > > + {"pktio mac", pktio_test_mac}, > > > This does not follow the existing naming style in this file ok. > > CU_TEST_INFO_NULL > }; > > > Nit Was it better to convert all the spaces to tabs? It looks like you > could have added your extra prints without converting them all to tabs > and thus reduced the size of the diff. Ok, I just followed original code. Maxim. > > > CU_SuiteInfo odp_testsuites[] = { > - {"odp_pktio", > + {"Packet I/O", > init_pktio_suite, term_pktio_suite, NULL, NULL, > pktio_tests}, > CU_SUITE_INFO_NULL > }; > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP
diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c index f7dc80b..4e3cc00 100644 --- a/test/validation/odp_pktio.c +++ b/test/validation/odp_pktio.c @@ -403,6 +403,74 @@ static void test_odp_pktio_sched_multi(void) pktio_test_txrx(ODP_QUEUE_TYPE_SCHED, 4); } +static void pktio_test_mtu(void) +{ + int ret; + int mtu; + odp_pktio_t pktio = create_pktio(iface_name[0]); + + mtu = odp_pktio_mtu(pktio); + CU_ASSERT(mtu > 0); + + printf(" %d ", mtu); + + ret = odp_pktio_close(pktio); + CU_ASSERT(ret == 0); + + return; +} + +static void pktio_test_promisc(void) +{ + int ret; + odp_pktio_t pktio = create_pktio(iface_name[0]); + + ret = odp_pktio_promisc_mode_set(pktio, 1); + CU_ASSERT(0 == ret); + + /* Check */ + ret = odp_pktio_promisc_mode(pktio); + CU_ASSERT(1 == ret); + + ret = odp_pktio_promisc_mode_set(pktio, 0); + CU_ASSERT(0 == ret); + + /* Check */ + ret = odp_pktio_promisc_mode(pktio); + CU_ASSERT(0 == ret); + + ret = odp_pktio_close(pktio); + CU_ASSERT(ret == 0); + + return; +} + +static void pktio_test_mac(void) +{ + unsigned char mac_addr[ODPH_ETHADDR_LEN]; + size_t mac_len; + int ret; + odp_pktio_t pktio = create_pktio(iface_name[0]); + + printf("testing mac for %s\n", iface_name[0]); + + mac_len = odp_pktio_mac_addr(pktio, mac_addr, ODPH_ETHADDR_LEN); + CU_ASSERT(ODPH_ETHADDR_LEN == mac_len); + + printf(" %X:%X:%X:%X:%X:%X ", + mac_addr[0], mac_addr[1], mac_addr[2], + mac_addr[3], mac_addr[4], mac_addr[5]); + + /* Fail case */ + mac_len = odp_pktio_mac_addr(pktio, mac_addr, 2); + CU_ASSERT(0 == mac_len); + + ret = odp_pktio_close(pktio); + CU_ASSERT(ret == 0); + + return; +} + static void test_odp_pktio_open(void) { odp_pktio_t pktio; @@ -483,19 +551,22 @@ static int term_pktio_suite(void) } CU_TestInfo pktio_tests[] = { - {"pktio open", test_odp_pktio_open}, - {"pktio close", test_odp_pktio_close}, - {"pktio inq", test_odp_pktio_inq}, - {"pktio outq", test_odp_pktio_outq}, - {"pktio poll queues", test_odp_pktio_poll_queue}, - {"pktio poll multi", test_odp_pktio_poll_multi}, - {"pktio sched queues", test_odp_pktio_sched_queue}, - {"pktio sched multi", test_odp_pktio_sched_multi}, + {"pktio open", test_odp_pktio_open}, + {"pktio close", test_odp_pktio_close}, + {"pktio inq", test_odp_pktio_inq}, + {"pktio outq", test_odp_pktio_outq}, + {"pktio poll queues", test_odp_pktio_poll_queue}, + {"pktio poll multi", test_odp_pktio_poll_multi}, + {"pktio sched queues", test_odp_pktio_sched_queue}, + {"pktio sched multi", test_odp_pktio_sched_multi}, + {"pktio mtu", pktio_test_mtu}, + {"pktio promisc mode", pktio_test_promisc}, + {"pktio mac", pktio_test_mac}, CU_TEST_INFO_NULL }; CU_SuiteInfo odp_testsuites[] = { - {"odp_pktio", + {"Packet I/O", init_pktio_suite, term_pktio_suite, NULL, NULL, pktio_tests}, CU_SUITE_INFO_NULL };
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- test/validation/odp_pktio.c | 89 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 80 insertions(+), 9 deletions(-)