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 DCD53A0471 for ; Wed, 17 Jul 2019 16:04:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EEA651BE0F; Wed, 17 Jul 2019 16:04:14 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id F0DE41BE05 for ; Wed, 17 Jul 2019 16:04:12 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1DBB4300CA84; Wed, 17 Jul 2019 14:04:12 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7260619C59; Wed, 17 Jul 2019 14:04:11 +0000 (UTC) From: Aaron Conole To: Stephen Hemminger Cc: "Li\, WenjieX A" , "Wang\, FengqinX" , "Byrne\, Stephen1" , dev@dpdk.org 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> Date: Wed, 17 Jul 2019 10:04:10 -0400 In-Reply-To: <20190716091704.2f9d0d01@hermes.lan> (Stephen Hemminger's message of "Tue, 16 Jul 2019 09:17:04 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Wed, 17 Jul 2019 14:04:12 +0000 (UTC) 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" 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 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