[libvirt] [PATCH/RFC]: hostdev passthrough support take #2

[Daniel P. Berrange]

On Sun, Aug 03, 2008 at 01:41:28AM +0200, Guido G?nther wrote:
> Hi,
> attached is a second version. Changes are:
> 
> * s/bus/subsystem/
> * support hexadecimal and decimal attributes
> * introduce device and source elements. 
> 
> I decided to not drop vendor and product id into their own elements
> since the structure would then become very nested for no good reason.

The reason is that I want it to match the host device enumeration
XML format. This will be using explicit <product> and <vendor>
tags, with an 'id' attribute, because there will be text content
in the body giving the human readable name.

> Some examples:
> 
>     <hostdev mode="subsys" type='usb'>
>       <source>
>         <device vendor="0x0204" product="0x6025"/>

So this needs to be changed to

          <vendor id="0x0204"/>
          <product id="0x6025"/>

>       </source>
>     </hostdev>
> 

>     <hostdev mode="subsys" type='usb'>
>       <source>
>         <address bus="001" device="003"/>
>       </source>
>     </hostdev>

This one is fine.

> 
> Support for <target> will be coming once I got around to adjust
> qemu/kvm.

Ok, that's not critical, but the other things we need before we
can commit this are

 - Code to format XML for output, so that the devices are included when
    dumping the XML description of a guest.
 - Update to the RNG schema in docs/libvirt.rng
 - Example XML data files in tests/qemuxml2argvdata/, along with the
   corresponding  CLI args for QEMU. THis is then hooked into the 
   qemuxml2argvtest.c and qemuxml2xmltest.c files

The latter test suite requires point the XML formatting & RNG updates.

The currrent code you have for parsing XML & attaching device/creating
CLI flags is basically  correct with only very minor bugs

>  
> +static int
> +virDomainHostdevSubsysUsbDefParseXML(virConnectPtr conn,
> +                                     const xmlNodePtr node,
> +				     virDomainHostdevDefPtr def) {

Minor indentation bug.

> +
> +    int ret = -1;
> +    xmlNodePtr cur;
> +
> +    def->source.subsys.usb.vendor = 0;
> +    def->source.subsys.usb.product = 0;
> +    def->source.subsys.usb.bus = 0;
> +    def->source.subsys.usb.device = 0;

This isn't needed, since the VIR_ALLOC contract says that the memory
range has been memset() to all zero.

> +static int qemudDomainAttachHostDevice(virDomainPtr dom, virDomainDeviceDefPtr dev)
> +{
> +    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
> +    virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> +    int ret;
> +    char *cmd, *reply;
> +
> +    if (dev->data.hostdev->source.subsys.usb.vendor) {
> +        ret = asprintf(&cmd, "usb_add host:%.4x:%.4x",
> +                       dev->data.hostdev->source.subsys.usb.vendor,
> +                       dev->data.hostdev->source.subsys.usb.product);
> +    } else {
> +        ret = asprintf(&cmd, "usb_add host:%.3d.%.3d",
> +                       dev->data.hostdev->source.subsys.usb.bus,
> +                       dev->data.hostdev->source.subsys.usb.device);
> +    }
> +    if (ret == -1) {
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> +                         "%s", _("out of memory"));
> +        return -1;

This should be reporting OOM, and not pass the 'dom' parameter, and
no need for a string message, eg

   qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL);

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|