Hiroyuki Kaguchi <fj7025cf@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; + }