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 CB831A0471 for ; Wed, 17 Jul 2019 16:39:48 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 13CEF1BDF1; Wed, 17 Jul 2019 16:39:48 +0200 (CEST) Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 998F11B9DF for ; Wed, 17 Jul 2019 16:39:46 +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 1hnl8s-00073G-IU; Wed, 17 Jul 2019 16:42:52 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Wed, 17 Jul 2019 16:39:38 +0200 Date: Wed, 17 Jul 2019 16:39:38 +0200 From: Olivier Matz To: Aaron Conole Cc: Stephen Hemminger , "Li, WenjieX A" , "Wang, FengqinX" , "Byrne, Stephen1" , dev@dpdk.org Message-ID: <20190717143938.3qogb4kygvxddwkq@platinum> References: <20190516180427.17270-1-stephen@networkplumber.org> <20190624204435.29452-1-stephen@networkplumber.org> <20190624204435.29452-7-stephen@networkplumber.org> <8688172CD5C0B74590FAE19D9579F94B53742875@SHSMSX103.ccr.corp.intel.com> <20190716091704.2f9d0d01@hermes.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180716 Subject: Re: [dpdk-dev] [PATCH v5 6/8] cmdline: use new ethernet address parser 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 Wed, Jul 17, 2019 at 10:04:10AM -0400, Aaron Conole wrote: > Stephen Hemminger writes: > > > On Tue, 16 Jul 2019 01:17:56 +0000 > > "Li, WenjieX A" wrote: > > > >> Hi Stephen, > >> > >> > >> > >> This DPDK patch makes cmdline_autotest failed. List the details as below. > >> > >> Could you please help to resolve it? > >> > >> Thank you! > >> Test Setup > >> Steps to reproduce > >> List the steps to reproduce the issue. > >> ./x86_64-native-linuxapp-gcc/app/test -n 1 -c 0xff > >> testpmd> cmdline_autotest > >> Show the output from the previous commands. > >> Testind parsing ethernet addresses... > >> Error: parsing 01:23:45:67:89:A succeeded! > >> Test Failed > >> Expected Result > >> Explain what is the expected result in text or as an example output: > >> Testind parsing ethernet addresses... > >> Testind parsing port lists... > >> Testind parsing numbers... > >> Testing parsing IP addresses... > >> Testing parsing strings... > >> Testing circular buffer... > >> Testing library functions... > >> Test OK > >> > >> > >> > >> BR, > >> > >> Wenjie > >> > > > > There are two possible solutions. Make the ether_unformat_addr function > > more complex and more restrictive. The new version corresponds to the FreeBSD > > behavior. > > Not exactly. The new code accepts 2 forms (X:X:X:X:X:X which is > FreeBSD, as well as X:X:X which is not). Because of this, there is a > higher likelihood of ambiguity. I want to submit a patch to fix this > ambiguity (but got pulled into a customer bug). Maybe someone else can? I will have a look at it. > I think it's best to make the code slightly more complex. I originally > submitted a change just validating the string length[0], which is probably > too restrictive. > > Maybe a better version would be something like the original (rewritten > to ensure it supports things like XX:X:XX:X:XX:X format, also): > > { > unsigned long o[ETHER_ADDR_LEN]; > char *end, *input = buf; > size_t i = 0; > > do { > errno = 0; > o[i] = strtoul(input, &end, 16); > if (errno != 0 || end == input || (end && *end != ':' && *end != 0)) > return ERROR; > input = end+1; > } while((input - buf < buflen) && ++i < ARRAY_SIZE(o) && *end != 0); > > if (*end != 0) > return ERROR; > > if (i == 6) /*x:x:x:x:x:x*/ > else if (i == 3) /*x:x:x*/ > else return ERROR > } > > WDYT? > > > The other possibility is just remove the test case. > > I don't like this approach - right now its possible that a user (doing > c/p) can submit an ipv6 address which will be accepted as valid. I > prefer to be validating the form. Even if XX:X:X:XX:X:X is okay, > XXXX:XXXX:XX::X shouldn't be. There's a limit to how liberal we should > be when accepting input. > > > I am leaning towards the latter as the least work. > > Least work isn't always the right thing. > > 0: http://mails.dpdk.org/archives/dev/2019-July/137827.html