From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 7517429C6 for ; Fri, 16 Nov 2018 11:35:37 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Nov 2018 02:35:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,239,1539673200"; d="scan'208";a="105113047" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.124]) ([10.237.220.124]) by fmsmga002.fm.intel.com with ESMTP; 16 Nov 2018 02:35:35 -0800 To: Li Han , reshma.pattan@intel.com Cc: dev@dpdk.org References: <1541571009-12396-1-git-send-email-han.li1@zte.com.cn> <3202c296-0bbf-1832-4552-bff912c93e06@intel.com> From: "Burakov, Anatoly" Message-ID: <6e90f28c-b84d-29fa-e152-c9a81e3bd0d3@intel.com> Date: Fri, 16 Nov 2018 10:35:35 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3202c296-0bbf-1832-4552-bff912c93e06@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3] app/proc-info: fix port mask parse issue 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: Fri, 16 Nov 2018 10:35:37 -0000 On 16-Nov-18 10:15 AM, Burakov, Anatoly wrote: > On 07-Nov-18 6:10 AM, Li Han wrote: >> parse_portmask return type is int,but global variable >> "enabled_port_mask" type is uint32_t.so in proc_info_parse_args >> function,when parse_portmask return -1,"enabled_port_mask" will >> get a huge value and "if (enabled_port_mask == 0)" will never happen. >> >> Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info") >> Signed-off-by: Li Han >> >> --- >> v3: >> *fix commit meassges issue >> v2: >> *fix typecast issue >> --- >>   app/proc-info/main.c | 9 +++------ >>   1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/app/proc-info/main.c b/app/proc-info/main.c >> index c20effa..650d599 100644 >> --- a/app/proc-info/main.c >> +++ b/app/proc-info/main.c >> @@ -37,7 +37,7 @@ >>   #define MAX_STRING_LEN 256 >>   /**< mask of enabled ports */ >> -static uint32_t enabled_port_mask; >> +static uint64_t enabled_port_mask; >>   /**< Enable stats. */ >>   static uint32_t enable_stats; >>   /**< Enable xstats. */ >> @@ -90,7 +90,7 @@ >>   /* >>    * Parse the portmask provided at run time. >>    */ >> -static int >> +static unsigned long >>   parse_portmask(const char *portmask) >>   { >>       char *end = NULL; >> @@ -103,12 +103,9 @@ >>       if ((portmask[0] == '\0') || (end == NULL) || (*end != '\0') || >>           (errno != 0)) { >>           printf("%s ERROR parsing the port mask\n", __func__); >> -        return -1; >> +        return 0; >>       } >> -    if (pm == 0) >> -        return -1; >> - >>       return pm; >>   } >> > > Hi, > > This fix appears wrong. If you're making the value uint64_t, you cannot > encode errors in the value. So, it's better to leave the return type as > int, return 0 or -1 on success/error, and store the parsed result in a > pointer passed to the function instead. Something like this: > > static int > parse_portmask(const char *portmask, uint64_t *mask) > { > ... >     if (pm == 0) >     return -1; >     *mask = pm; >     return 0; > } > Also, another thing to note. Unsigned long is 32-bit on 32-bit Linux, so if you're going to have uint64_t data, you should correct the parsing to use strtoull() instead. -- Thanks, Anatoly