[v5,6/8] cmdline: use new ethernet address parser
Checks
Commit Message
Now that there is a version of ether_aton in rte_ether, it can
be used by the cmdline ethernet address parser.
Note: ether_aton_r can not be used in cmdline because
the old code would accept either bytes XX:XX:XX:XX:XX:XX
or words XXXX:XXXX:XXXX and we need to keep compatiablity.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
lib/Makefile | 2 +-
lib/librte_cmdline/Makefile | 3 +-
lib/librte_cmdline/cmdline_parse_etheraddr.c | 71 +++-----------------
lib/librte_cmdline/meson.build | 6 +-
lib/meson.build | 2 +-
5 files changed, 15 insertions(+), 69 deletions(-)
Comments
On Tue, 16 Jul 2019 01:17:56 +0000
"Li, WenjieX A" <wenjiex.a.li@intel.com> 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.
The other possibility is just remove the test case.
I am leaning towards the latter as the least work.
Stephen Hemminger <stephen@networkplumber.org> writes:
> On Tue, 16 Jul 2019 01:17:56 +0000
> "Li, WenjieX A" <wenjiex.a.li@intel.com> 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
On Wed, Jul 17, 2019 at 10:04:10AM -0400, Aaron Conole wrote:
> Stephen Hemminger <stephen@networkplumber.org> writes:
>
> > On Tue, 16 Jul 2019 01:17:56 +0000
> > "Li, WenjieX A" <wenjiex.a.li@intel.com> 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
@@ -20,7 +20,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_TIMER) += librte_timer
DEPDIRS-librte_timer := librte_eal
DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile
DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
-DEPDIRS-librte_cmdline := librte_eal
+DEPDIRS-librte_cmdline := librte_eal librte_net
DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ethdev
DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
DEPDIRS-librte_ethdev += librte_mbuf
@@ -7,6 +7,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
LIB = librte_cmdline.a
CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
+CFLAGS += -DALLOW_EXPERIMENTAL_API
EXPORT_MAP := rte_cmdline_version.map
@@ -25,7 +26,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) += cmdline_vt100.c
SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) += cmdline_socket.c
SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) += cmdline_parse_portlist.c
-LDLIBS += -lrte_eal
+LDLIBS += -lrte_net -lrte_eal
# install includes
INCS := cmdline.h cmdline_parse.h cmdline_parse_num.h cmdline_parse_ipaddr.h
@@ -12,9 +12,9 @@
#include <ctype.h>
#include <string.h>
#include <sys/types.h>
-#include <net/ethernet.h>
#include <rte_string_fns.h>
+#include <rte_ether.h>
#include "cmdline_parse.h"
#include "cmdline_parse_etheraddr.h"
@@ -26,69 +26,15 @@ struct cmdline_token_ops cmdline_token_etheraddr_ops = {
.get_help = cmdline_get_help_etheraddr,
};
-/* the format can be either XX:XX:XX:XX:XX:XX or XXXX:XXXX:XXXX */
-#define ETHER_ADDRSTRLENLONG 18
-#define ETHER_ADDRSTRLENSHORT 15
-
-#ifdef __linux__
-#define ea_oct ether_addr_octet
-#else
-#define ea_oct octet
-#endif
-
-
-static struct ether_addr *
-my_ether_aton(const char *a)
-{
- int i;
- char *end;
- unsigned long o[ETHER_ADDR_LEN];
- static struct ether_addr ether_addr;
-
- i = 0;
- do {
- errno = 0;
- o[i] = strtoul(a, &end, 16);
- if (errno != 0 || end == a || (end[0] != ':' && end[0] != 0))
- return NULL;
- a = end + 1;
- } while (++i != sizeof (o) / sizeof (o[0]) && end[0] != 0);
-
- /* Junk at the end of line */
- if (end[0] != 0)
- return NULL;
-
- /* Support the format XX:XX:XX:XX:XX:XX */
- if (i == ETHER_ADDR_LEN) {
- while (i-- != 0) {
- if (o[i] > UINT8_MAX)
- return NULL;
- ether_addr.ea_oct[i] = (uint8_t)o[i];
- }
- /* Support the format XXXX:XXXX:XXXX */
- } else if (i == ETHER_ADDR_LEN / 2) {
- while (i-- != 0) {
- if (o[i] > UINT16_MAX)
- return NULL;
- ether_addr.ea_oct[i * 2] = (uint8_t)(o[i] >> 8);
- ether_addr.ea_oct[i * 2 + 1] = (uint8_t)(o[i] & 0xff);
- }
- /* unknown format */
- } else
- return NULL;
-
- return (struct ether_addr *)ðer_addr;
-}
-
int
cmdline_parse_etheraddr(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
const char *buf, void *res, unsigned ressize)
{
unsigned int token_len = 0;
- char ether_str[ETHER_ADDRSTRLENLONG+1];
- struct ether_addr *tmp;
+ char ether_str[RTE_ETHER_ADDR_FMT_SIZE];
+ struct rte_ether_addr tmp;
- if (res && ressize < sizeof(struct ether_addr))
+ if (res && ressize < sizeof(tmp))
return -1;
if (!buf || ! *buf)
@@ -98,17 +44,16 @@ cmdline_parse_etheraddr(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
token_len++;
/* if token doesn't match possible string lengths... */
- if ((token_len != ETHER_ADDRSTRLENLONG - 1) &&
- (token_len != ETHER_ADDRSTRLENSHORT - 1))
+ if (token_len >= RTE_ETHER_ADDR_FMT_SIZE)
return -1;
strlcpy(ether_str, buf, token_len + 1);
- tmp = my_ether_aton(ether_str);
- if (tmp == NULL)
+ if (rte_ether_unformat_addr(ether_str, &tmp) < 0)
return -1;
+
if (res)
- memcpy(res, tmp, sizeof(struct ether_addr));
+ memcpy(res, &tmp, sizeof(tmp));
return token_len;
}
@@ -1,10 +1,8 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright(c) 2017 Intel Corporation
-# This library is processed before EAL
-includes = [global_inc]
-
version = 2
+allow_experimental_apis = true
sources = files('cmdline.c',
'cmdline_cirbuf.c',
'cmdline_parse.c',
@@ -28,3 +26,5 @@ headers = files('cmdline.h',
'cmdline_socket.h',
'cmdline_cirbuf.h',
'cmdline_parse_portlist.h')
+
+deps += ['net']
@@ -11,8 +11,8 @@
libraries = [
'kvargs', # eal depends on kvargs
'eal', # everything depends on eal
- 'cmdline', # ethdev depends on cmdline for parsing functions
'ring', 'mempool', 'mbuf', 'net', 'meter', 'ethdev', 'pci', # core
+ 'cmdline',
'metrics', # bitrate/latency stats depends on this
'hash', # efd depends on this
'timer', # eventdev depends on this