[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Thanks very much! Still two places to confirm:

2013/8/21 Daniel P. Berrange <berrange redhat com>
On Mon, Aug 19, 2013 at 04:49:37PM -0400, 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?
 
> +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 

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]