Thanks very much! Still two places to confirm: 2013/8/21 Daniel P. Berrange <[email protected]>
> On Mon, Aug 19, 2013 at 04:49:37PM -0400, [email protected] 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? > > +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. > > diff --git a/src/util/virpci.c b/src/util/virpci.c > > index be50b4f..dc38efe 100644 > > --- a/src/util/virpci.c > > +++ b/src/util/virpci.c > > @@ -62,7 +62,10 @@ struct _virPCIDevice { > > char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ > > char id[PCI_ID_LEN]; /* product vendor */ > > char *path; > > - const char *used_by; /* The domain which uses the > device */ > > + > > + /* The driver:domain which uses the device */ > > + const char *used_by_drvname; > > + const char *used_by_domname; > > [snip] > > > > void > > -virPCIDeviceSetUsedBy(virPCIDevicePtr dev, const char *name) > > +virPCIDeviceSetUsedBy(virPCIDevicePtr dev, const char *drv_name, const > char *dom_name) > > { > > - dev->used_by = name; > > + dev->used_by_drvname = drv_name; > > + dev->used_by_domname = dom_name; > > I know you are just following existing code design, but I consider it > pretty bad practice to store pointers to parameters that are passed > in. You can never be sure when someone is using to use this API > in the future with a string that they free sooner than we expect. > > Much much safer to strdup the parameters. > > > > } > > > > -const char * > > -virPCIDeviceGetUsedBy(virPCIDevicePtr dev) > > +int > > +virPCIDeviceGetUsedBy(virPCIDevicePtr dev, char **drv_name, char > **dom_name) > > { > > - return dev->used_by; > > + if (VIR_STRDUP(*drv_name, dev->used_by_drvname) < 0) > > + return -1; > > + if (VIR_STRDUP(*dom_name, dev->used_by_domname) < 0) > > + return -1; > > Strdup'ing the parameters in a getter method is odd practice > and this method didn't do that before. > > > diff --git a/src/util/virscsi.c b/src/util/virscsi.c > > index 32e438b..dc1eebb 100644 > > --- a/src/util/virscsi.c > > +++ b/src/util/virscsi.c > > @@ -55,7 +55,10 @@ struct _virSCSIDevice { > > char *name; /* adapter:bus:target:unit */ > > char *id; /* model:vendor */ > > char *sg_path; /* e.g. /dev/sg2 */ > > - const char *used_by; /* name of the domain using this dev */ > > + > > + /* driver:domain using this dev */ > > + const char *used_by_drvname; > > + const char *used_by_domname; > > > > bool readonly; > > }; > > @@ -267,15 +270,22 @@ virSCSIDeviceFree(virSCSIDevicePtr dev) > > > > void > > virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, > > - const char *name) > > + const char *drvname, > > + const char *domname) > > { > > - dev->used_by = name; > > + dev->used_by_drvname = drvname; > > + dev->used_by_domname = domname; > > } > > Same comment as previous method. > > > void virUSBDeviceSetUsedBy(virUSBDevicePtr dev, > > - const char *name) > > + const char *drv_name, > > + const char *dom_name) > > { > > - dev->used_by = name; > > + dev->used_by_drvname = drv_name; > > + dev->used_by_domname = dom_name; > > } > > And again > > > > > -const char * virUSBDeviceGetUsedBy(virUSBDevicePtr dev) > > +int virUSBDeviceGetUsedBy(virUSBDevicePtr dev, > > + char **drv_name, > > + char **dom_name) > > { > > - return dev->used_by; > > + if (VIR_STRDUP(*drv_name, dev->used_by_drvname) < 0) > > + return -1; > > + if (VIR_STRDUP(*dom_name, dev->used_by_domname) < 0) > > + return -1; > > + > > + return 0; > > } > > And again > > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/:| > |: http://libvirt.org -o- http://virt-manager.org:| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc >
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
