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 <[email protected]
<mailto:[email protected]>> wrote:
Signed-off-by: Maxim Uvarov <[email protected]
<mailto:[email protected]>>
---
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
[email protected] <mailto:[email protected]>
http://lists.linaro.org/mailman/listinfo/lng-odp
--
*Mike Holmes*
Linaro Sr Technical Manager
LNG - ODP
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
