Chunyan Liu wrote:
Thanks very much! Still two places to confirm:
2013/8/21 Daniel P. Berrange <berrange@redhat.com <mailto:berrange@redhat.com>>
On Mon, Aug 19, 2013 at 04:49:37PM -0400, cyliu@suse.com <mailto:cyliu@suse.com> wrote: > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c > new file mode 100644 > index 0000000..1baa829 > --- /dev/null > +++ b/src/util/virhostdev.c > + > +/* For virReportOOMError() and virReportSystemError() */
No need for this comment - this is standard practice for every source file
> +#define VIR_FROM_THIS VIR_FROM_NONE
> + > +/* Same name as in virDriver. For special check. */ > +#define LIBXL_DRIVER_NAME "xenlight" > +#define QEMU_DRIVER_NAME "QEMU"
You're using this later to determine whether to use pci-back vs pci-stub.
I think it it would be preferrable to have the drivers pass in the name of their desired stub driver instead.
I'm afraid there are some problems: Currently there are two places: 1. if <driver name=xx /> is not specified in pci hostdev .xml, use default stub driver. But to 'libxl' and 'qemu', the default stub driver is different (pciback vs pci-stub), so, need to check hypervisor driver name to decide default stub dirver name. 2. in detach-device, for 'qemu/kvm', it needs to check 'kvm_assigned_device', waiting for device cleanup. For 'libxl', it doesn't. So, need to check hypervisor driver name to add the extra processing.
Besides, to 'qemu', not only the 'pci-stub' case, it could be 'pci-stub' or 'vfio'. To prepare domain hostdevs, just could not pass ONE stub driver name simply to virHostdev API (e.g, virHostdevPrepareDomainHostdevs).
Any suggestions?
Perhaps these driver-specific items could be handled by callbacks into the driver, similar to driver-specific XML parsing and formating callbacks in the common domain_conf code.
> +static int > +virHostdevOnceInit(void) > +{ > + char ebuf[1024]; > + > + if (VIR_ALLOC(hostdevMgr) < 0) > + goto error; > + > + if ((hostdevMgr->activePciHostdevs = virPCIDeviceListNew()) == NULL) > + goto error; > + > + if ((hostdevMgr->activeUsbHostdevs = virUSBDeviceListNew()) == NULL) > + goto error; > + > + if ((hostdevMgr->inactivePciHostdevs = virPCIDeviceListNew()) == NULL) > + goto error; > + > + if ((hostdevMgr->activeScsiHostdevs = virSCSIDeviceListNew()) == NULL) > + goto error; > + > + if (virAsprintf(&hostdevMgr->stateDir, "%s", HOSTDEV_STATE_DIR) < 0) > + goto error; > + > + if (virFileMakePath(hostdevMgr->stateDir) < 0) { > + VIR_ERROR(_("Failed to create state dir '%s': %s"), > + hostdevMgr->stateDir, virStrerror(errno, ebuf, sizeof(ebuf)));
You should be using virReportError here
> + goto error; > + } > + > + return 0; > + > +error:
You should free the partially initialized 'hostdevMgr' instance & all its data
> + return -1; > +} > + > +VIR_ONCE_GLOBAL_INIT(virHostdev) > + > +virHostdevManagerPtr > +virHostdevManagerGetDefault(void) > +{ > + virHostdevInitialize();
You should do
if (virHostdevInitialize() < 0) return NULL;
> + return hostdevMgr; > +} > +
> + > +void > +virHostdevReAttachUsbHostdevs(virHostdevManagerPtr mgr, > + const char *drv_name, > + const char *dom_name, > + virDomainHostdevDefPtr *hostdevs, > + int nhostdevs) > +{ > + size_t i; > + > + virObjectLock(mgr->activeUsbHostdevs);
I wonder if we should get rid of the mutex locks in the PCI / USB device list objects, and instead have a single lock on the virHostdevManagerPtr instance.
I noticed in qemu_hostdev.c, originally it used single driver lock, later changed to use pci/usb list object lock. Do you think a single lock is still preferred? If yes, I'll update.
I don't know the reason for that change, but it would seem desirable to manage USB devices without needing to wait for some operation on a PCI device to complete. Regards, Jim