6:36 a.m.
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(a)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;
}
9:55 a.m.
Hiroyuki Kaguchi <fj7025cf(a)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;
}
8:07 a.m.
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(a)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;
}
10:37 a.m.
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
11:55 a.m.
Hiroyuki Kaguchi <fj7025cf(a)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;
+ }
12:25 p.m.
On Wed, Mar 19, 2008 at 11:55:54AM +0100, Jim Meyering wrote:
Hiroyuki Kaguchi <fj7025cf(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
3:47 a.m.
Hi, Jim
Thank you very much for your comments.
Regards
Hiroyuki Kaguchi
On 2008/03/19 19:55, Jim Meyering wrote:
Hiroyuki Kaguchi <fj7025cf(a)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;
> + }
10:58 a.m.
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
6027
days inactive
6030
days old
7 comments
4 participants
participants (4)
-
Daniel Veillard -
Hiroyuki Kaguchi -
Jim Meyering -
Richard W.M. Jones