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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/