On Tue, Mar 13, 2007 at 05:50:38PM +0900, Masayuki Sunou wrote:
Hi Daniel
I examined your suggestions, and corrected the patch as follows. ・ only (usb 1) <usb> <bus/> </usb>
・ (usbdevice mouse) <usb> <mouse/> </usb>
・ (usbdevice tablet) <usb> <tablet/> </usb>
・ (usbdevice disk:xxx) <usb> <disk> <source dev='xxx'/> </disk> </usb>
・ (usbdevice host:xxx.yyy) <usb> <host bus='xxx' addr='yyy'/> </usb>
・ (usbdevice host:xxx:yyy) <usb> <host vendor='xxx' product='yyy'/> </usb>
okay that looks fine to me
Can we just share parsing or is the fact that a disk hooked on USB bus such a real difference ?
I think that dividing usual disk and disk of USB is necessary. Because USB enables a dynamic detaching of disk though usual disk cannot do it by domHVM.
okay, fair enough.
+ offset = strtok((char *)tmp, ":");
Sorry you can't use strtok there, it's not reentrant, uses a global variable and as a result multiple concurent threads accessing libvirt for different connections may get their data randomly corrupted, this is a big problem. I prefer parsing to be done explicitely computing indexes from the string being parsed (and then you can use strndup() to generate copies if needed).
+ if ((offset = strtok(NULL, ":")) != NULL) { + virBufferAdd(&buf, " <disk>\n", 13 ); + virBufferVSprintf(&buf, " <source dev='%s'/>\n", offset); + virBufferAdd(&buf, " </disk>\n", 14 ); + }
plus offset would not been freed here, so that would be a memory leak too,
+ } else if (strncmp(tmp, "host:", 5) == 0) { + if (strrchr(tmp, ':') > (tmp + 4)) { + offset = strtok((char *)tmp, ":"); + if ((offset = strtok(NULL, ":")) != NULL) { + virBufferVSprintf(&buf, " <host vendor='%s'", offset); + if ((offset = strtok(NULL, ":")) != NULL) { + virBufferVSprintf(&buf, " product='%s'", offset); + } + virBufferAdd(&buf, "/>\n", 3 ); + } + } else { + offset = strtok((char *)tmp, ":."); + if ((offset = strtok(NULL, ":.")) != NULL) { + virBufferVSprintf(&buf, " <host bus='%s'", offset); + if ((offset = strtok(NULL, ":.")) != NULL) { + virBufferVSprintf(&buf, " addr='%s'", offset); + } + virBufferAdd(&buf, "/>\n", 3 ); + } + } + } else if (strncmp(tmp, "mouse", 5) == 0) { + virBufferAdd(&buf, " <mouse/>\n", 15 ); + } else if (strncmp(tmp, "tablet", 6) == 0) { + virBufferAdd(&buf, " <tablet/>\n", 16 ); + } + virBufferAdd(&buf, " </usb>\n", 11 ); + } else if (sexpr_int(root, "domain/image/hvm/usb")) { + virBufferAdd(&buf, " <usb>\n", 10 ); + virBufferAdd(&buf, " <bus/>\n", 13 ); + virBufferAdd(&buf, " </usb>\n", 11 ); + } + virBufferAdd(&buf, " </devices>\n", 13); virBufferAdd(&buf, "</domain>\n", 10);
Index: src/xml.c =================================================================== RCS file: /data/cvs/libvirt/src/xml.c,v retrieving revision 1.64 diff -u -p -r1.64 xml.c
the patch for xml.c looks good to me, but I would rather apply both at the same time. Could you regenerate a patch doing the same but avoiding strtok (I'm not too fond of strtok_r() either). thanks in advance ! 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/