[U-Boot] [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address

[Joe Hershberger]

Hi York,

On Mon, Aug 10, 2015 at 3:03 PM, York Sun <yorksun at freescale.com> wrote:
>
>
> On 08/10/2015 12:57 PM, Joe Hershberger wrote:
>> Too much top-posting.
>>
>> On Mon, Aug 10, 2015 at 2:41 PM, York Sun <yorksun at freescale.com> wrote:
>>> SPL doesn't use net/eth.c. You add a call in env_flags.c.
>>>
>>> I think you can put it in header file and use static inline, or keep it in the
>>> same file where it is called.
>>
>> That is probably fine.
>>
>>> Another way is to undef CONFIG_CMD_NET for SPL part. It is default to 'y' in
>>> Kconfig. Joe may have some good suggestion.
>>
>> I don't think this is the reason. The problem is that net is *not*
>> build for SPL, but env is.
>
> Yes, env is built. The offending lines in common/env_flags.c are gated by
> "#ifdef CONFIG_CMD_NET". That's why I say it could be another way.

OK, sure... but that breaks intended behavior, I think.

>>
>>> On 08/10/2015 01:44 AM, Ciubotariu Codrin Constantin-B43658 wrote:
>>>> Hi York,
>>>>
>>>> I didn't know that SPL uses net/eth.c . Could you please suggest a place for eth_validate_ethaddr_str()?
>>>>
>>>> Thanks and best regards,
>>>> Codrin
>>>>
>>>>> -----Original Message-----
>>>>> From: Sun York-R58495
>>>>> Sent: Saturday, August 08, 2015 3:31 AM
>>>>> To: Ciubotariu Codrin Constantin-B43658; u-boot at lists.denx.de
>>>>> Cc: joe.hershberger at ni.com; Kushwaha Prabhakar-B32579
>>>>> Subject: Re: [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address
>>>>>
>>>>> On 07/24/2015 06:55 AM, Codrin Ciubotariu wrote:
>>>>>> The code from common/env_flags.c that checks if a string has the
>>>>>> format of a MAC address has been moved in net/eth.c as a separate
>>>>>> function called eth_validate_ethaddr_str().
>>>>>>
>>>>>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu at freescale.com>
>>>>>> ---
>>>>>>
>>>>>> Changes for v3:
>>>>>>     - none, new patch;
>>>>>>
>>>>>>  common/env_flags.c | 15 ++-------------
>>>>>>  include/net.h      |  1 +
>>>>>>  net/eth.c          | 30 ++++++++++++++++++++++++++++++
>>>>>>  3 files changed, 33 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/common/env_flags.c b/common/env_flags.c index
>>>>>> 5189f5b..3e39fd1 100644
>>>>>> --- a/common/env_flags.c
>>>>>> +++ b/common/env_flags.c
>>>>>> @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value,
>>>>>>             }
>>>>>>             break;
>>>>>>     case env_flags_vartype_macaddr:
>>>>>> -           cur = value;
>>>>>> -           for (i = 0; i < 6; i++) {
>>>>>> -                   skip_num(1, cur, &end, 2);
>>>>>> -                   if (cur == end)
>>>>>> -                           return -1;
>>>>>> -                   if (cur + 2 == end && is_hex_prefix(cur))
>>>>>> -                           return -1;
>>>>>> -                   if (i != 5 && *end != ':')
>>>>>> -                           return -1;
>>>>>> -                   if (i == 5 && *end != '\0')
>>>>>> -                           return -1;
>>>>>> -                   cur = end + 1;
>>>>>> -           }
>>>>>> +           if (eth_validate_ethaddr_str(value))
>>>>>> +                   return -1;
>>>>>>             break;
>>>>>>  #endif
>>>>>>     case env_flags_vartype_end:
>>>>>> diff --git a/include/net.h b/include/net.h index d17173d..c487aa7
>>>>>> 100644
>>>>>> --- a/include/net.h
>>>>>> +++ b/include/net.h
>>>>>> @@ -218,6 +218,7 @@ void eth_try_another(int first_restart);        /* Change the
>>>>> device */
>>>>>>  void eth_set_current(void);                /* set nterface to ethcur var */
>>>>>>
>>>>>>  int eth_get_dev_index(void);               /* get the device index */
>>>>>> +int eth_validate_ethaddr_str(const char *addr);
>>>>>>  void eth_parse_enetaddr(const char *addr, uchar *enetaddr);  int
>>>>>> eth_getenv_enetaddr(char *name, uchar *enetaddr);  int
>>>>>> eth_setenv_enetaddr(char *name, const uchar *enetaddr); diff --git
>>>>>> a/net/eth.c b/net/eth.c index 953b731..a6fdf1b 100644
>>>>>> --- a/net/eth.c
>>>>>> +++ b/net/eth.c
>>>>>> @@ -7,6 +7,7 @@
>>>>>>   */
>>>>>>
>>>>>>  #include <common.h>
>>>>>> +#include <linux/ctype.h>
>>>>>>  #include <command.h>
>>>>>>  #include <dm.h>
>>>>>>  #include <environment.h>
>>>>>> @@ -19,6 +20,35 @@
>>>>>>
>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>
>>>>>> +int eth_validate_ethaddr_str(const char *addr) {
>>>>>> +   unsigned long val;
>>>>>> +   int i;
>>>>>> +   const char *cur;
>>>>>> +   char *end;
>>>>>> +
>>>>>> +   if (!addr)
>>>>>> +           return -1;
>>>>>> +
>>>>>> +   cur = addr;
>>>>>> +   for (i = 0; i < 6; i++) {
>>>>>> +           val = simple_strtoul(cur, &end, 16);
>>>>>> +           if (cur + 1 != end && cur + 2 != end)
>>>>>> +                   return -1;
>>>>>> +           if (val > 0xff)
>>>>>> +                   return -1;
>>>>>> +           if (cur + 2 >= end && tolower(*(cur + 1)) == 'x')
>>>>>> +                   return -1;
>>>>>> +           if (i != 5 && *end != ':')
>>>>>> +                   return -1;
>>>>>> +           if (i == 5 && *end != '\0')
>>>>>> +                   return -1;
>>>>>> +           cur = end + 1;
>>>>>> +   }
>>>>>> +
>>>>>> +   return 0;
>>>>>> +}
>>>>>> +
>>>>>>  void eth_parse_enetaddr(const char *addr, uchar *enetaddr)  {
>>>>>>     char *end;
>>>>>>
>>>>>
>>>>> Codrin,
>>>>>
>>>>> This patch breaks most SPL targets. Please reconsider the location of
>>>>> eth_validate_ethaddr_str().
>>>>>
>>>>> York
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot