From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 670D21BB4B for ; Thu, 5 Jul 2018 16:05:46 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jul 2018 07:05:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,312,1526367600"; d="scan'208";a="243215176" Received: from rhorton-mobl1.ger.corp.intel.com (HELO [163.33.178.248]) ([163.33.178.248]) by fmsmga006.fm.intel.com with ESMTP; 05 Jul 2018 07:05:44 -0700 To: Hari kumar Vemula References: <1530776231-17707-1-git-send-email-hari.kumarx.vemula@intel.com> Cc: reshma.pattan@intel.com, dev@dpdk.org From: Remy Horton Organization: Intel Shannon Limited Message-ID: <92bd80fd-e1cc-4928-72e8-015dd218517d@intel.com> Date: Thu, 5 Jul 2018 15:05:43 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <1530776231-17707-1-git-send-email-hari.kumarx.vemula@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] test: add unit tests for metrics library 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: , X-List-Received-Date: Thu, 05 Jul 2018 14:05:47 -0000 'noon, See inline comments. On 05/07/2018 08:37, Hari kumar Vemula wrote: > Unit Testcases are added for metrics library. [..] > +/* Test case to validate registering a list of valid metric names */ > +static int > +test_metrics_reg_names(void) > +{ > + int err = 0; > + const char * const mnames[] = { > + "mean_bits_in", "mean_bits_out", > + "peak_bits_in", "peak_bits_out", > + }; > + > + > + /* Success Test: valid array and count size */ > + err = rte_metrics_reg_names(&mnames[0], ARRAY_SIZE(mnames)); > + TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__); > + > + /* Failure Test: valid array and higher count size than array size*/ > + err = rte_metrics_reg_names(&mnames[0], ARRAY_SIZE(mnames) + 2); > + TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__); rte_metrics_reg_names() relies on cnt_names being truthful about the size of *names. If it is overstated there is nothing to prevent an array overrun and in most cases a core-dump. > + > + /* Failure Test: valid array and count size lessthan 1*/ > + err = rte_metrics_reg_names(&mnames[0], 0); > + TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__); > + > + /* Failure Test: valid array and count exceeds max count */ > + err = rte_metrics_reg_names(&mnames[0], INVALID_COUNT); > + TEST_ASSERT(err == -ENOMEM, "%s, %d", __func__, __LINE__); See previous comment. > + > + /* Failure Test: Invalid array and valid count size */ > + err = rte_metrics_reg_names(NULL, ARRAY_SIZE(mnames) + 2); > + TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__); > + > + /* Failure Test: Valid array and Invalid count size */ > + err = rte_metrics_reg_names(&mnames[0], INVAL_METRIC_COUNT); > + TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__); Ditto. > + > + return TEST_SUCCESS; > +} > + [..] > +/* Test case to validate update a list of metrics */ > +static int > +test_metrics_update_values(void) > +{ > + int err = 0; > + const uint64_t value[REG_METRIC_COUNT] = {1, 2, 3, 4, 5, 6}; > + > + /* Success Test: valid data */ > + err = rte_metrics_update_values(RTE_METRICS_GLOBAL, > + KEY, &value[0], ARRAY_SIZE(value)); > + TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__); Updates cannot overlap more than one metric set. A successful test would be: /* Success Test: valid data */ err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 0, &value[0], 1); TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__); err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 1, &value[1], 1); TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__); err = rte_metrics_update_values(RTE_METRICS_GLOBAL, 2, &value[2], 4); TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__); I am wondering whether this constraint should be removed. > + /* Failed Test: Invalid count size */ > + err = rte_metrics_update_values(RTE_METRICS_GLOBAL, > + KEY, &value[0], 0); > + TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__); Good catch. Will update library code. > + > + /* Success Test: valid data with lower count stats size */ > + err = rte_metrics_update_values(RTE_METRICS_GLOBAL, > + KEY, &value[0], ARRAY_SIZE(value) - 3); > + TEST_ASSERT(err >= 0, "%s, %d", __func__, __LINE__); KEY should be 2, which is where the set of size four starts. The set starting at KEY (i.e 1) is of size one. > + > + /* Failed Test: Invalid port_id(lower value) and valid data */ > + err = rte_metrics_update_values(-2, KEY, &value[0], ARRAY_SIZE(value)); > + TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__); > + > + /* Failed Test: Invalid port_id(higher value) and valid data */ > + err = rte_metrics_update_values(39, 1, &value[0], ARRAY_SIZE(value)); > + TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__); Testcases Ok. > + > + /* Failed Test: higher count size */ > + err = rte_metrics_update_values(RTE_METRICS_GLOBAL, > + KEY, &value[0], ARRAY_SIZE(value) + 5); > + TEST_ASSERT(err == -ERANGE, "%s, %d", __func__, __LINE__); Array sizes should not be overstated. > + > + /* Failed Test: Invalid array */ > + err = rte_metrics_update_values(RTE_METRICS_GLOBAL, > + KEY, NULL, ARRAY_SIZE(value)); > + TEST_ASSERT(err == -EINVAL, "%s, %d", __func__, __LINE__); > + > + return TEST_SUCCESS; > +} > + > +/* Test to validate get metric name-key lookup table */ > +static int > +test_metrics_get_names(void) > +{ [..] > + > + /* Failure Test: Invalid array list, Correct Count Stats same as > + * memzone stats > + */ > + err = rte_metrics_get_names(metrics, REG_METRIC_COUNT); > + TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__); Overstated array size. > + > + /* Failure Test: Invalid array list, Increased Count Stats */ > + err = rte_metrics_get_names(metrics, REG_METRIC_COUNT+9); > + TEST_ASSERT(err < 0, "%s, %d", __func__, __LINE__); Overstated array size. > + > + return TEST_SUCCESS; > +} > + > +/* Test to validate get list of metric values */ ..Remy