[Libvir] [PATCH] Enable USB device setting information handling on virsh.

[Daniel Veillard]

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