[Libvir] [PATCH] Fix MAC address parsing for 1-digit case

[Jim Meyering]

Hiroyuki Kaguchi <fj7025cf at aa.jp.fujitsu.com> wrote:
> Thank you for your suggestions.
> I apply the following changes to the patch.
> 1) Initialize errno to 0 before the loop.
> 2) Use isxdigit instead of isdigit and isalpha, and add a comment.
> 3) Test errno != 0.
> 4) Add a terminating NUL check.
> 5) the MAC address function go in file src/util.c.

Looks good.  A couple nits.
I didn't read the comments the first time.

> +/**
> + * virParseMacAddr:
> + * @str: pointer to the char used
> + * @addr: MAC address

 * @str: string representation of MAC address, e.g., "0:1E:FC:E:3a:CB"
 * @addr: 6-byte MAC address

> + * Parse a MAC address
> + *
> + * Returns 0 or -1 in case of error.

 * Return 0 upon success, or -1 in case of error.

> + */
> +int
> +virParseMacAddr(const char* str, unsigned char *addr)
> +{
> +    int i;
> +
> +    errno = 0;
> +    for (i = 0; i < 6; i++) {
> +        char *end_ptr;
> +        unsigned long result;
> +
> +        /* This is solely to avoid accepting the leading
> +         * space or "+" that strtoul would otherwise accept.
> +         */
> +        if (!isxdigit(*str))
> +            break;
> +
> +        result = strtoul(str, &end_ptr, 16);
> +
> +        if ((end_ptr - str) < 1 || 2 < (end_ptr - str) ||
> +            (errno != 0) ||
> +            (0xFF < result))
> +            break;
> +
> +        addr[i] = (unsigned char) result;
> +
> +        if (*end_ptr != ':') {
> +            if (i == 5 && *end_ptr == '\0')
> +                return 0;
> +
> +            break;
> +        }

No big deal, but I find this to be more readable:

           if (i == 5 && *end_ptr == '\0')
               return 0;
           if (*end_ptr != ':')
               break;

and with any compiler optimization at all, it's just as efficient.

> +        str = end_ptr + 1;
> +    }