From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2E04CA0613 for ; Mon, 26 Aug 2019 11:09:25 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 292C51BF3E; Mon, 26 Aug 2019 11:09:24 +0200 (CEST) Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 5BB9C1BF37 for ; Mon, 26 Aug 2019 11:09:22 +0200 (CEST) Received: from lfbn-lil-1-176-160.w90-45.abo.wanadoo.fr ([90.45.26.160] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1i2B3O-0000vF-QO; Mon, 26 Aug 2019 11:12:48 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Mon, 26 Aug 2019 11:09:19 +0200 Date: Mon, 26 Aug 2019 11:09:19 +0200 From: Olivier Matz To: Lavanya Govindarajan Cc: dev@dpdk.org, reshma.pattan@intel.com, bruce.richardson@intel.com, pallantlax.poornima@intel.com Message-ID: <20190826090919.xicdbb7p63iwiwgr@platinum> References: <1563884086-30759-2-git-send-email-lavanyax.govindarajan@intel.com> <1565271260-10295-3-git-send-email-lavanyax.govindarajan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1565271260-10295-3-git-send-email-lavanyax.govindarajan@intel.com> User-Agent: NeoMutt/20180716 Subject: Re: [dpdk-dev] [PATCH v4 2/2] app/test: add unit test cases to mbuf X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Aug 08, 2019 at 02:34:20PM +0100, Lavanya Govindarajan wrote: > From: Pallantla Poornima > > Added UT for the below four functions in test_mbuf.c > rte_get_rx_ol_flag_list > rte_get_tx_ol_flag_list > rte_get_rx_ol_flag_name > rte_get_tx_ol_flag_name > > Signed-off-by: Pallantla Poornima I suggest to change the patch title from "app/test: add unit test cases to mbuf" to "app/test: add unit test for mbuf flag names" > --- > app/test/test_mbuf.c | 260 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 260 insertions(+) > > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c > index 28f3216c0..1a943518d 100644 > --- a/app/test/test_mbuf.c > +++ b/app/test/test_mbuf.c > @@ -42,6 +42,7 @@ > #define MBUF_TEST_DATA_LEN3 256 > #define MBUF_TEST_HDR1_LEN 20 > #define MBUF_TEST_HDR2_LEN 30 > +#define MBUF_TEST_LEN 250 > #define MBUF_TEST_ALL_HDRS_LEN (MBUF_TEST_HDR1_LEN+MBUF_TEST_HDR2_LEN) > #define MBUF_TEST_SEG_SIZE 64 > #define MBUF_TEST_BURST 8 > @@ -1132,6 +1133,245 @@ test_tx_offload(void) > return (v1 == v2) ? 0 : -EINVAL; > } > > + > +static int > +test_get_rx_ol_flag_list(void) > +{ > + int len, ret = 0; > + char buf[256] = ""; > + int buflen = 0; > + > + /* Test case to check with null buffer */ > + ret = rte_get_rx_ol_flag_list(0, NULL, 0); > + if (ret != -1) > + GOTO_FAIL("%s expected: -1, received = %d\n", __func__, ret); > + > + /* Test case to check with zero buffer len */ > + ret = rte_get_rx_ol_flag_list(PKT_RX_L4_CKSUM_MASK, buf, 0); > + if (ret != -1) > + GOTO_FAIL("%s expected: -1, received = %d\n", __func__, ret); > + > + buflen = strlen(buf); > + if (buflen != 0) > + GOTO_FAIL("%s buffer should be empty, received = %d\n", > + __func__, buflen); > + > + /* Test case to check with reduced buffer len */ > + len = sizeof(buf) - MBUF_TEST_LEN; > + ret = rte_get_rx_ol_flag_list(0, buf, len); Why using a #define for MBUF_TEST_LEN and not for char buf[256]? Also, MBUF_TEST_LEN is not a very clear name. So, I'd prefer to have an hardcoded value: len = 5; ret = rte_get_rx_ol_flag_list(0, buf, len); > + if (ret != -1) > + GOTO_FAIL("%s expected: -1, received = %d\n", __func__, ret); > + > + buflen = strlen(buf); > + if (buflen != (len - 1)) > + GOTO_FAIL("%s invalid buffer length retrieved, expected: %d," > + "received = %d\n", __func__, > + (len - 1), buflen); > + > + /* Test case to check with zero mask value */ > + ret = rte_get_rx_ol_flag_list(0, buf, sizeof(buf)); > + if (ret != 0) > + GOTO_FAIL("%s expected: 0, received = %d\n", __func__, ret); > + > + buflen = strlen(buf); > + if (buflen == 0) > + GOTO_FAIL("%s expected: %s, received length = 0\n", __func__, > + "non-zero, buffer should not be empty"); > + > + /* Test case to check with valid mask value */ > + ret = rte_get_rx_ol_flag_list(PKT_RX_SEC_OFFLOAD, buf, sizeof(buf)); > + if (ret != 0) > + GOTO_FAIL("%s expected: 0, received = %d\n", __func__, ret); > + > + buflen = strlen(buf); > + if (buflen == 0) > + GOTO_FAIL("%s expected: %s, received length = 0\n", __func__, > + "non-zero, buffer should not be empty"); > + > + > + return 0; > +fail: > + return -1; > +} > + > +static int > +test_get_tx_ol_flag_list(void) > +{ Same comment as rx. [...] > +struct flag_name { > + uint64_t flag; > + const char *name; > +}; > + > +static int > +test_get_rx_ol_flag_name(void) > +{ > + uint16_t i; > + const char *flag_str = NULL; > + const struct flag_name rx_flags[] = { > + { PKT_RX_VLAN, "PKT_RX_VLAN" }, > + { PKT_RX_RSS_HASH, "PKT_RX_RSS_HASH" }, > + { PKT_RX_FDIR, "PKT_RX_FDIR"}, > + { PKT_RX_L4_CKSUM_BAD, "PKT_RX_L4_CKSUM_BAD"}, > + { PKT_RX_L4_CKSUM_GOOD, "PKT_RX_L4_CKSUM_GOOD"}, > + { PKT_RX_L4_CKSUM_NONE, "PKT_RX_L4_CKSUM_NONE"}, > + { PKT_RX_IP_CKSUM_BAD, "PKT_RX_IP_CKSUM_BAD"}, > + { PKT_RX_IP_CKSUM_GOOD, "PKT_RX_IP_CKSUM_GOOD"}, > + { PKT_RX_IP_CKSUM_NONE, "PKT_RX_IP_CKSUM_NONE"}, > + { PKT_RX_EIP_CKSUM_BAD, "PKT_RX_EIP_CKSUM_BAD" }, > + { PKT_RX_VLAN_STRIPPED, "PKT_RX_VLAN_STRIPPED" }, > + { PKT_RX_IEEE1588_PTP, "PKT_RX_IEEE1588_PTP"}, > + { PKT_RX_IEEE1588_TMST, "PKT_RX_IEEE1588_TMST"}, > + { PKT_RX_FDIR_ID, "PKT_RX_FDIR_ID"}, > + { PKT_RX_FDIR_FLX, "PKT_RX_FDIR_FLX"}, > + { PKT_RX_QINQ_STRIPPED, "PKT_RX_QINQ_STRIPPED" }, > + { PKT_RX_LRO, "PKT_RX_LRO" }, > + { PKT_RX_TIMESTAMP, "PKT_RX_TIMESTAMP"}, > + { PKT_RX_SEC_OFFLOAD, "PKT_RX_SEC_OFFLOAD" }, > + { PKT_RX_SEC_OFFLOAD_FAILED, "PKT_RX_SEC_OFFLOAD_FAILED" }, > + { PKT_RX_OUTER_L4_CKSUM_BAD, "PKT_RX_OUTER_L4_CKSUM_BAD" }, > + { PKT_RX_OUTER_L4_CKSUM_GOOD, "PKT_RX_OUTER_L4_CKSUM_GOOD"}, > + { PKT_RX_OUTER_L4_CKSUM_INVALID, > + "PKT_RX_OUTER_L4_CKSUM_INVALID" }, > + }; Since flag value and name are the same, why not using a #define to ensure there is no typo? Something like this: #define VAL_NAME(flag) { flag, #flag } const struct flag_name rx_flags[] = { VAL_NAME(PKT_RX_VLAN), VAL_NAME(PKT_RX_RSS_HASH), ... It makes me think that the same thing could be done in rte_mbuf.c instead... in this case the test would become overkill. [...] > +static int > +test_get_tx_ol_flag_name(void) > +{ Same comment as rx. Thanks, Olivier