[libvirt] [PATCHv4 1/4] add hostdev passthrough common library
Chunyan Liu
cyliu at suse.com
Thu Aug 22 08:19:26 UTC 2013
Thanks very much! Still two places to confirm:
2013/8/21 Daniel P. Berrange <berrange at redhat.com>
> On Mon, Aug 19, 2013 at 04:49:37PM -0400, cyliu at 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?
> > +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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130822/2c297a63/attachment-0001.htm>
More information about the libvir-list
mailing list