[Libvir] [PATCH] Fix MAC address parsing for 1-digit case
libvirt fails in parsing when MAC address like "00:16:3e:12:3:61" is specified for installation. This is because virt-install can pass 1-digit (like 3) for MAC address from Cset:316 for Solaris But libvirt cannot support this MAC 1-digit (like 3) parameter. This patch fixes libvirt parsing in the MAC address against virt-inst Cset:316 Thanks Signed-off-by: Hiroyuki Kaguchi <fj7025cf@aa.jp.fujitsu.com> ? parse_macaddr.patch Index: src/xml.c =================================================================== RCS file: /data/cvs/libvirt/src/xml.c,v retrieving revision 1.113 diff -u -r1.113 xml.c --- src/xml.c 27 Feb 2008 04:35:08 -0000 1.113 +++ src/xml.c 18 Mar 2008 05:30:14 -0000 @@ -17,6 +17,7 @@ #include <string.h> #include <stdarg.h> #include <limits.h> +#include <ctype.h> #ifdef WITH_XEN #include <xs.h> #endif @@ -537,6 +538,44 @@ ************************************************************************/ #if WITH_XEN /** + * parseMacAddr: + * @str: mac addrress string + * @addr: mac addrress numbers + * + * Parse a mac addrress + * + * Returns 0 in case success or -1 in case of error. + */ +static int +parseMacAddr(const char* str, unsigned char *addr) +{ + int i; + for (i = 0; i < 6; i++) { + char *end_ptr; + unsigned long result; + + if (!isdigit(*str) && !isalpha(*str)) + break; + + result = strtoul(str, &end_ptr, 16); + + if ((end_ptr - str) < 1 || 2 < (end_ptr - str) || + (errno == ERANGE) || + (0xFF < result)) + break; + + addr[i] = (unsigned char) result; + + if (*end_ptr != ':') + return (i == 5) ? 0 : -1; + + str = end_ptr + 1; + } + + return -1; +} + +/** * virtDomainParseXMLGraphicsDescImage: * @conn: pointer to the hypervisor connection * @node: node containing graphics description @@ -1233,22 +1272,8 @@ virBufferAddLit(buf, "(vif "); if (mac != NULL) { - unsigned int addr[12]; - int tmp = sscanf((const char *) mac, - "%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x", - (unsigned int *) &addr[0], - (unsigned int *) &addr[1], - (unsigned int *) &addr[2], - (unsigned int *) &addr[3], - (unsigned int *) &addr[4], - (unsigned int *) &addr[5], - (unsigned int *) &addr[6], - (unsigned int *) &addr[7], - (unsigned int *) &addr[8], - (unsigned int *) &addr[9], - (unsigned int *) &addr[10], - (unsigned int *) &addr[11]); - if (tmp != 12 || strlen((const char *) mac) != 17) { + unsigned char addr[6]; + if (parseMacAddr((const char*) mac, addr) == -1) { virXMLError(conn, VIR_ERR_INVALID_MAC, (const char *) mac, 0); goto error; }
Hiroyuki Kaguchi <fj7025cf@aa.jp.fujitsu.com> wrote:
libvirt fails in parsing when MAC address like "00:16:3e:12:3:61" is specified for installation. This is because virt-install can pass 1-digit (like 3) for MAC address from Cset:316 for Solaris But libvirt cannot support this MAC 1-digit (like 3) parameter.
Thanks for the patch. Any change that eliminates a sscanf-based parser is a good one ;-) A few suggestions: ...
+static int +parseMacAddr(const char* str, unsigned char *addr) +{ + int i;
Initialize errno to 0 before the loop. Otherwise, an incoming nonzero value could cause rejection of a valid MAC address.
+ for (i = 0; i < 6; i++) { + char *end_ptr; + unsigned long result; + + if (!isdigit(*str) && !isalpha(*str)) + break;
Use isxdigit instead, and add a comment saying 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 == ERANGE) ||
While it's ok to test errno == ERANGE, testing errno != 0 is a little better.
+ (0xFF < result)) + break; + + addr[i] = (unsigned char) result;
The following test ensures that there is a ":" between adjacent pairs, but does not require a terminating NUL byte. That means it would accept arbitrary text after a valid sixth component, e.g., "00:16:3e:12:3:61-anything-at-all" or a less-contrived s/0/O/ typo, e.g., "00:16:3e:12:3:6O"
+ if (*end_ptr != ':') + return (i == 5) ? 0 : -1;
You can use this instead: if (*end_ptr != (i < 5 ? ':' : '\0')) return -1; if (i == 5) return 0; ...
virBufferAddLit(buf, "(vif "); if (mac != NULL) { - unsigned int addr[12]; - int tmp = sscanf((const char *) mac, - "%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x", - (unsigned int *) &addr[0], - (unsigned int *) &addr[1], - (unsigned int *) &addr[2], - (unsigned int *) &addr[3], - (unsigned int *) &addr[4], - (unsigned int *) &addr[5], - (unsigned int *) &addr[6], - (unsigned int *) &addr[7], - (unsigned int *) &addr[8], - (unsigned int *) &addr[9], - (unsigned int *) &addr[10], - (unsigned int *) &addr[11]); - if (tmp != 12 || strlen((const char *) mac) != 17) { + unsigned char addr[6]; + if (parseMacAddr((const char*) mac, addr) == -1) { virXMLError(conn, VIR_ERR_INVALID_MAC, (const char *) mac, 0); goto error; }
Hi, Jim and Rich 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. Thanks, Signed-off-by: Hiroyuki Kaguchi <fj7025cf@aa.jp.fujitsu.com> Index: src/util.c =================================================================== RCS file: /data/cvs/libvirt/src/util.c,v retrieving revision 1.25 diff -u -r1.25 util.c --- src/util.c 3 Mar 2008 20:39:52 -0000 1.25 +++ src/util.c 19 Mar 2008 06:29:15 -0000 @@ -688,6 +688,53 @@ return (c > d ? 1 : c < d ? -1 : 0); } +/** + * virParseMacAddr: + * @str: pointer to the char used + * @addr: MAC address + * + * Parse a MAC address + * + * Returns 0 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; + } + + str = end_ptr + 1; + } + + return -1; +} + /* * Local variables: * indent-tabs-mode: nil Index: src/util.h =================================================================== RCS file: /data/cvs/libvirt/src/util.h,v retrieving revision 1.13 diff -u -r1.13 util.h --- src/util.h 27 Feb 2008 16:14:44 -0000 1.13 +++ src/util.h 19 Mar 2008 06:29:15 -0000 @@ -85,4 +85,6 @@ void virSkipSpaces(const char **str); int virParseNumber(const char **str); +int virParseMacAddr(const char* str, unsigned char *addr); + #endif /* __VIR_UTIL_H__ */ Index: src/xml.c =================================================================== RCS file: /data/cvs/libvirt/src/xml.c,v retrieving revision 1.113 diff -u -r1.113 xml.c --- src/xml.c 27 Feb 2008 04:35:08 -0000 1.113 +++ src/xml.c 19 Mar 2008 06:29:16 -0000 @@ -1233,22 +1233,8 @@ virBufferAddLit(buf, "(vif "); if (mac != NULL) { - unsigned int addr[12]; - int tmp = sscanf((const char *) mac, - "%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x:%01x%01x", - (unsigned int *) &addr[0], - (unsigned int *) &addr[1], - (unsigned int *) &addr[2], - (unsigned int *) &addr[3], - (unsigned int *) &addr[4], - (unsigned int *) &addr[5], - (unsigned int *) &addr[6], - (unsigned int *) &addr[7], - (unsigned int *) &addr[8], - (unsigned int *) &addr[9], - (unsigned int *) &addr[10], - (unsigned int *) &addr[11]); - if (tmp != 12 || strlen((const char *) mac) != 17) { + unsigned char addr[6]; + if (virParseMacAddr((const char*) mac, addr) == -1) { virXMLError(conn, VIR_ERR_INVALID_MAC, (const char *) mac, 0); goto error; }
On Wed, Mar 19, 2008 at 04:07:34PM +0900, Hiroyuki Kaguchi wrote:
Hi, Jim and Rich
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.
Yup, can't see anything wrong in that patch. +1 Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
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; + }
On Wed, Mar 19, 2008 at 11:55:54AM +0100, Jim Meyering wrote:
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.
Okay, I have commited the suggested patch after applying the changes proposed by Jim, thanks a lot ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi, Jim Thank you very much for your comments. Regards Hiroyuki Kaguchi On 2008/03/19 19:55, Jim Meyering wrote:
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; + }
On Tue, Mar 18, 2008 at 02:36:02PM +0900, Hiroyuki Kaguchi wrote:
Index: src/xml.c [...] + * parseMacAddr:
Since all hypervisors support MAC addresses, and we already have a shared MAC address function (__virMacAddrCompare), this should go in file src/util.c. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
participants (4)
-
Daniel Veillard -
Hiroyuki Kaguchi -
Jim Meyering -
Richard W.M. Jones