Re: [libvirt] [PATCHv4 1/4] add hostdev passthrough common library

On Mon, Aug 19, 2013 at 04:49:37PM -0400, cyliu(a)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.

in the name of their desired stub driver instead.

> +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

> 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