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.

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

> 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