2014-02-17 16:31 GMT+08:00 Cedric Bosdonnat <cbosdonnat@suse.com>:
Hello ChunYan,
Good to see your patchset into smaller pieces. This patch looks almost OK for me, but there are problems you'll need to look into.
Add driver info to used_by, to avoid conflict among different drivers if
On Mon, 2014-02-17 at 14:32 +0800, Chunyan Liu wrote: there
are more than one drivers existing and using the hostdev.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/lxc/lxc_hostdev.c | 11 +++++++---- src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_hostdev.c | 41 +++++++++++++++++++++++++---------------- src/util/virpci.c | 30 +++++++++++++++++++++++------- src/util/virpci.h | 9 ++++++--- src/util/virscsi.c | 30 ++++++++++++++++++++++-------- src/util/virscsi.h | 7 +++++-- src/util/virusb.c | 29 ++++++++++++++++++++++------- src/util/virusb.h | 8 ++++++-- tests/virscsitest.c | 6 +++--- 11 files changed, 125 insertions(+), 56 deletions(-)
diff --git a/src/lxc/lxc_hostdev.c b/src/lxc/lxc_hostdev.c index 3b371fc..77ce965 100644 --- a/src/lxc/lxc_hostdev.c +++ b/src/lxc/lxc_hostdev.c @@ -60,7 +60,7 @@ virLXCUpdateActiveUsbHostdevs(virLXCDriverPtr driver, continue; }
- virUSBDeviceSetUsedBy(usb, def->name); + virUSBDeviceSetUsedBy(usb, "QEMU", def->name);
You meant LXC_DRIVER_NAME rather than "QEMU" here, right?
virObjectLock(driver->activeUsbHostdevs); if (virUSBDeviceListAdd(driver->activeUsbHostdevs, usb) < 0) { @@ -90,7 +90,9 @@ virLXCPrepareHostdevUSBDevices(virLXCDriverPtr driver, for (i = 0; i < count; i++) { virUSBDevicePtr usb = virUSBDeviceListGet(list, i); if ((tmp = virUSBDeviceListFind(driver->activeUsbHostdevs,
usb))) {
- const char *other_name = virUSBDeviceGetUsedBy(tmp); + const char *other_name = NULL; + const char *other_drvname = NULL; + virUSBDeviceGetUsedBy(tmp, &other_drvname, &other_name);
if (other_name) virReportError(VIR_ERR_OPERATION_INVALID, @@ -103,7 +105,7 @@ virLXCPrepareHostdevUSBDevices(virLXCDriverPtr driver, goto error; }
- virUSBDeviceSetUsedBy(usb, name); + virUSBDeviceSetUsedBy(usb, "QEMU", name);
Same here, I guess that should be LXC_DRIVER_NAME, no?
VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", virUSBDeviceGetBus(usb), virUSBDeviceGetDevno(usb),
name);
/* @@ -352,6 +354,7 @@ virLXCDomainReAttachHostUsbDevices(virLXCDriverPtr
driver,
virDomainHostdevDefPtr hostdev = hostdevs[i]; virUSBDevicePtr usb, tmp; const char *used_by = NULL; + const char *used_by_drvname = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) continue; @@ -389,7 +392,7 @@ virLXCDomainReAttachHostUsbDevices(virLXCDriverPtr
driver,
continue; }
- used_by = virUSBDeviceGetUsedBy(tmp); + virUSBDeviceGetUsedBy(tmp, &used_by_drvname, &used_by);
Where is used_by_drvname used? If that really isn't used, then replacing it by NULL would be fine.
if (STREQ_NULLABLE(used_by, name)) { VIR_DEBUG("Removing %03d.%03d dom=%s from
activeUsbHostdevs",
hostdev->source.subsys.u.usb.bus, diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1f44a76..158cc1a 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -53,6 +53,8 @@ # error "Port me" # endif
+# define QEMU_DRIVER_NAME "QEMU" + typedef struct _virQEMUDriver virQEMUDriver; typedef virQEMUDriver *virQEMUDriverPtr;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 59e018d..1fe8992 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -97,8 +97,6 @@
#define VIR_FROM_THIS VIR_FROM_QEMU
-#define QEMU_DRIVER_NAME "QEMU" - #define QEMU_NB_MEM_PARAM 3
#define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 @@ -11325,7 +11323,9 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) virObjectLock(driver->inactivePciHostdevs); other = virPCIDeviceListFind(driver->activePciHostdevs, pci); if (other) { - const char *other_name = virPCIDeviceGetUsedBy(other); + const char *other_name = NULL; + const char *other_drvname = NULL; + virPCIDeviceGetUsedBy(other, &other_drvname, &other_name);
Is other_drvname used any here? Seems not from the patch, so using
NULLwould surely be preferable. Not used in this patch since tmp_drvname check is always 'success' within the qemu code. It will be used when switching to using common library. I'll change it.
if (other_name) virReportError(VIR_ERR_OPERATION_INVALID, @@ -16684,7 +16684,7 @@ static virDriver qemuDriver = {
static virStateDriver qemuStateDriver = { - .name = "QEMU", + .name = QEMU_DRIVER_NAME, .stateInitialize = qemuStateInitialize, .stateAutoStart = qemuStateAutoStart, .stateCleanup = qemuStateCleanup, diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 1b16386..01c24f9 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -177,7 +177,7 @@ qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver, goto cleanup;
} - virPCIDeviceSetUsedBy(dev, def->name); + virPCIDeviceSetUsedBy(dev, QEMU_DRIVER_NAME, def->name);
/* Setup the original states for the PCI device */ virPCIDeviceSetUnbindFromStub(dev,
hostdev->origstates.states.pci.unbind_from_stub);
@@ -230,7 +230,7 @@ qemuUpdateActiveUsbHostdevs(virQEMUDriverPtr driver, continue; }
- virUSBDeviceSetUsedBy(usb, def->name); + virUSBDeviceSetUsedBy(usb, QEMU_DRIVER_NAME, def->name);
if (virUSBDeviceListAdd(driver->activeUsbHostdevs, usb) < 0) { virUSBDeviceFree(usb); @@ -274,13 +274,13 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, goto cleanup;
if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) { - if (virSCSIDeviceSetUsedBy(tmp, def->name) < 0) { + if (virSCSIDeviceSetUsedBy(tmp, QEMU_DRIVER_NAME, def->name) < 0) { virSCSIDeviceFree(scsi); goto cleanup; } virSCSIDeviceFree(scsi); } else { - if (virSCSIDeviceSetUsedBy(scsi, def->name) < 0 || + if (virSCSIDeviceSetUsedBy(scsi, QEMU_DRIVER_NAME, def->name) < 0 || virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) { virSCSIDeviceFree(scsi); goto cleanup; @@ -693,7 +693,9 @@ qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, * the dev is in list driver->activePciHostdevs. */ if ((other = virPCIDeviceListFind(driver->activePciHostdevs, dev))) { - const char *other_name = virPCIDeviceGetUsedBy(other); + const char *other_name = NULL; + const char *other_drvname = NULL; + virPCIDeviceGetUsedBy(other, &other_drvname, &other_name);
Again, other_drvname seems to be unused.
if (other_name) virReportError(VIR_ERR_OPERATION_INVALID, @@ -766,7 +768,7 @@ qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, activeDev = virPCIDeviceListFind(driver->activePciHostdevs,
dev);
if (activeDev) - virPCIDeviceSetUsedBy(activeDev, name); + virPCIDeviceSetUsedBy(activeDev, QEMU_DRIVER_NAME, name); }
/* Loop 8: Now set the original states for hostdev def */ @@ -857,7 +859,9 @@ qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver, for (i = 0; i < count; i++) { virUSBDevicePtr usb = virUSBDeviceListGet(list, i); if ((tmp = virUSBDeviceListFind(driver->activeUsbHostdevs,
usb))) {
- const char *other_name = virUSBDeviceGetUsedBy(tmp); + const char *other_name = NULL; + const char *other_drvname = NULL; + virUSBDeviceGetUsedBy(tmp, &other_drvname, &other_name);
Here too.
if (other_name) virReportError(VIR_ERR_OPERATION_INVALID, @@ -870,7 +874,7 @@ qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver, goto error; }
- virUSBDeviceSetUsedBy(usb, name); + virUSBDeviceSetUsedBy(usb, QEMU_DRIVER_NAME, name); VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", virUSBDeviceGetBus(usb), virUSBDeviceGetDevno(usb),
name);
/* @@ -1140,10 +1144,10 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr
driver,
goto error; }
- if (virSCSIDeviceSetUsedBy(tmp, name) < 0) + if (virSCSIDeviceSetUsedBy(tmp, QEMU_DRIVER_NAME, name) < 0) goto error; } else { - if (virSCSIDeviceSetUsedBy(scsi, name) < 0) + if (virSCSIDeviceSetUsedBy(scsi, QEMU_DRIVER_NAME, name) <
0)
goto error;
VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
@@ -1275,10 +1279,14 @@ qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver, * been used by this domain. */ activeDev = virPCIDeviceListFind(driver->activePciHostdevs, dev); - if (activeDev && - STRNEQ_NULLABLE(name, virPCIDeviceGetUsedBy(activeDev))) { - virPCIDeviceListDel(pcidevs, dev); - continue; + if (activeDev) { + const char *tmp_name = NULL; + const char *tmp_drvname = NULL; + virPCIDeviceGetUsedBy(activeDev, &tmp_drvname, &tmp_name);
Is tmp_drvname actually used?
+ if (STRNEQ_NULLABLE(name, tmp_name)) { + virPCIDeviceListDel(pcidevs, dev); + continue; + } }
virPCIDeviceListDel(driver->activePciHostdevs, dev); @@ -1333,6 +1341,7 @@ qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev = hostdevs[i]; virUSBDevicePtr usb, tmp; const char *used_by = NULL; + const char *used_by_drvname = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) continue; @@ -1370,7 +1379,7 @@ qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver, continue; }
- used_by = virUSBDeviceGetUsedBy(tmp); + virUSBDeviceGetUsedBy(tmp, &used_by_drvname, &used_by);
Same here, used_by_drvname isn't used.
if (STREQ_NULLABLE(used_by, name)) { VIR_DEBUG("Removing %03d.%03d dom=%s from
activeUsbHostdevs",
hostdev->source.subsys.u.usb.bus, @@ -1445,7 +1454,7 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr
driver,
hostdev->source.subsys.u.scsi.unit, name);
- virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp, name); + virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp,
QEMU_DRIVER_NAME, name);
virSCSIDeviceFree(scsi); } virObjectUnlock(driver->activeScsiHostdevs); diff --git a/src/util/virpci.c b/src/util/virpci.c index 00d1064..4ea2bcf 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -59,7 +59,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 */ + char *used_by_drvname; + char *used_by_domname;
Why changing from const char* to char*?
used_by_domname is better to be char *, we can never be sure when someone uses this this API with a string that they free sooner than we expect. Much safer to strdup the parameter. And used_by_drvname is just a parallel. Will check and correct the memory leak. .
unsigned int pcie_cap_pos; unsigned int pci_pm_cap_pos; @@ -1635,6 +1638,8 @@ virPCIDeviceFree(virPCIDevicePtr dev) VIR_DEBUG("%s %s: freeing", dev->id, dev->name); VIR_FREE(dev->path); VIR_FREE(dev->stubDriver); + VIR_FREE(dev->used_by_drvname); + VIR_FREE(dev->used_by_domname); VIR_FREE(dev); }
@@ -1704,16 +1709,27 @@ virPCIDeviceSetReprobe(virPCIDevicePtr dev, bool
reprobe)
dev->reprobe = reprobe; }
-void -virPCIDeviceSetUsedBy(virPCIDevicePtr dev, const char *name) +int +virPCIDeviceSetUsedBy(virPCIDevicePtr dev, + const char *drv_name, + const char *dom_name) { - dev->used_by = name; + if (VIR_STRDUP(dev->used_by_drvname, drv_name) < 0) + return -1; + if (VIR_STRDUP(dev->used_by_domname, dom_name) < 0) + return -1; +
You will leak memory here if there was already a value set, You need to free dev->used_by_drvname and dev->used_by_domname before VIR_STRDUP'ing to them.
However, it would be much more convenient to keep const char*.
+ return 0; }
-const char * -virPCIDeviceGetUsedBy(virPCIDevicePtr dev) +void +virPCIDeviceGetUsedBy(virPCIDevicePtr dev, + const char **drv_name, + const char **dom_name) { - return dev->used_by; + *drv_name = dev->used_by_drvname; + *dom_name = dev->used_by_domname; }
void virPCIDeviceReattachInit(virPCIDevicePtr pci) diff --git a/src/util/virpci.h b/src/util/virpci.h index ac6dae1..20ffe54 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -66,9 +66,12 @@ int virPCIDeviceSetStubDriver(virPCIDevicePtr dev, const char *driver) ATTRIBUTE_NONNULL(2); const char *virPCIDeviceGetStubDriver(virPCIDevicePtr dev); -void virPCIDeviceSetUsedBy(virPCIDevice *dev, - const char *used_by); -const char *virPCIDeviceGetUsedBy(virPCIDevice *dev); +int virPCIDeviceSetUsedBy(virPCIDevice *dev, + const char *drv_name, + const char *dom_name); +void virPCIDeviceGetUsedBy(virPCIDevice *dev, + const char **drv_name, + const char **dom_name); unsigned int virPCIDeviceGetUnbindFromStub(virPCIDevicePtr dev); void virPCIDeviceSetUnbindFromStub(virPCIDevice *dev, bool unbind); diff --git a/src/util/virscsi.c b/src/util/virscsi.c index acc3815..5e28328 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -47,6 +47,11 @@
/* For virReportOOMError() and virReportSystemError() */ #define VIR_FROM_THIS VIR_FROM_NONE +struct _virUsedByInfo { + char *drvname; /* which driver */ + char *domname; /* which domain */ +}; +typedef struct _virUsedByInfo *virUsedByInfoPtr;
struct _virSCSIDevice { unsigned int adapter; @@ -57,7 +62,7 @@ struct _virSCSIDevice { char *name; /* adapter:bus:target:unit */ char *id; /* model:vendor */ char *sg_path; /* e.g. /dev/sg2 */ - char **used_by; /* name of the domains using this dev */ + virUsedByInfoPtr *used_by; /* driver:domain(s) using this dev */ size_t n_used_by; /* how many domains are using this dev */
bool readonly; @@ -274,19 +279,26 @@ virSCSIDeviceFree(virSCSIDevicePtr dev) VIR_FREE(dev->id); VIR_FREE(dev->name); VIR_FREE(dev->sg_path); - for (i = 0; i < dev->n_used_by; i++) + for (i = 0; i < dev->n_used_by; i++) { + VIR_FREE(dev->used_by[i]->drvname); + VIR_FREE(dev->used_by[i]->domname); VIR_FREE(dev->used_by[i]); + } VIR_FREE(dev->used_by); VIR_FREE(dev); }
int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, - const char *name) + const char *drvname, + const char *domname) { - char *copy = NULL; - - if (VIR_STRDUP(copy, name) < 0) + virUsedByInfoPtr copy; + if (VIR_ALLOC(copy) < 0) + return -1; + if (VIR_STRDUP(copy->drvname, drvname) < 0) + return -1; + if (VIR_STRDUP(copy->domname, domname) < 0) return -1;
return VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy); @@ -427,13 +439,15 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list, void virSCSIDeviceListDel(virSCSIDeviceListPtr list, virSCSIDevicePtr dev, - const char *name) + const char *drvname, + const char *domname) { virSCSIDevicePtr tmp = NULL; size_t i;
for (i = 0; i < dev->n_used_by; i++) { - if (STREQ_NULLABLE(dev->used_by[i], name)) { + if (STREQ_NULLABLE(dev->used_by[i]->drvname, drvname) && + STREQ_NULLABLE(dev->used_by[i]->domname, domname)) { if (dev->n_used_by > 1) { VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by);
I'ld say that doesn't free everything. You'll need to VIR_FREE the dev->drvname and dev->domname before.
} else { diff --git a/src/util/virscsi.h b/src/util/virscsi.h index 1b685eb..c67837f 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -53,7 +53,9 @@ virSCSIDevicePtr virSCSIDeviceNew(const char
*sysfs_prefix,
bool shareable);
void virSCSIDeviceFree(virSCSIDevicePtr dev); -int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name); +int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, + const char *drvname, + const char *domname); bool virSCSIDeviceIsAvailable(virSCSIDevicePtr dev); const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); @@ -87,7 +89,8 @@ virSCSIDevicePtr
virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev); void virSCSIDeviceListDel(virSCSIDeviceListPtr list, virSCSIDevicePtr dev, - const char *name); + const char *drvname, + const char *domname); virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list, virSCSIDevicePtr dev);
diff --git a/src/util/virusb.c b/src/util/virusb.c index bb5980d..4e22973 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -55,7 +55,10 @@ struct _virUSBDevice { char name[USB_ADDR_LEN]; /* domain:bus:slot.function */ char id[USB_ID_LEN]; /* product vendor */ char *path; - const char *used_by; /* name of the domain using this
dev */
+ + /* driver:domain using this dev */ + char *used_by_drvname; + char *used_by_domname; };
Again, why changing from const char* to char*? It will mostly lead to memory handling troubles.
struct _virUSBDeviceList { @@ -375,19 +378,31 @@ virUSBDeviceFree(virUSBDevicePtr dev) return; VIR_DEBUG("%s %s: freeing", dev->id, dev->name); VIR_FREE(dev->path); + VIR_FREE(dev->used_by_drvname); + VIR_FREE(dev->used_by_domname); VIR_FREE(dev); }
- -void virUSBDeviceSetUsedBy(virUSBDevicePtr dev, - const char *name) +int +virUSBDeviceSetUsedBy(virUSBDevicePtr dev, + const char *drv_name, + const char *dom_name) { - dev->used_by = name; + if (VIR_STRDUP(dev->used_by_drvname, drv_name) < 0) + return -1; + if (VIR_STRDUP(dev->used_by_domname, dom_name) < 0) + return -1;
What if there was already values set? There would be a memory leak: either use const char* or VIR_FREE before the VIR_STRDUP.
+ + return 0; }
-const char * virUSBDeviceGetUsedBy(virUSBDevicePtr dev) +void +virUSBDeviceGetUsedBy(virUSBDevicePtr dev, + const char **drv_name, + const char **dom_name) { - return dev->used_by; + *drv_name = dev->used_by_drvname; + *dom_name = dev->used_by_domname; }
const char *virUSBDeviceGetName(virUSBDevicePtr dev) diff --git a/src/util/virusb.h b/src/util/virusb.h index e0a7c4c..f98ea21 100644 --- a/src/util/virusb.h +++ b/src/util/virusb.h @@ -60,8 +60,12 @@ int virUSBDeviceFind(unsigned int vendor, virUSBDevicePtr *usb);
void virUSBDeviceFree(virUSBDevicePtr dev); -void virUSBDeviceSetUsedBy(virUSBDevicePtr dev, const char *name); -const char *virUSBDeviceGetUsedBy(virUSBDevicePtr dev); +int virUSBDeviceSetUsedBy(virUSBDevicePtr dev, + const char *drv_name, + const char *dom_name); +void virUSBDeviceGetUsedBy(virUSBDevicePtr dev, + const char **drv_name, + const char **dom_name); const char *virUSBDeviceGetName(virUSBDevicePtr dev);
unsigned int virUSBDeviceGetBus(virUSBDevicePtr dev); diff --git a/tests/virscsitest.c b/tests/virscsitest.c index d4b3e4a..586c41b 100644 --- a/tests/virscsitest.c +++ b/tests/virscsitest.c @@ -91,13 +91,13 @@ test2(const void *data ATTRIBUTE_UNUSED) if (!virSCSIDeviceIsAvailable(dev)) goto cleanup;
- if (virSCSIDeviceSetUsedBy(dev, "fc18") < 0) + if (virSCSIDeviceSetUsedBy(dev, "QEMU", "fc18") < 0) goto cleanup;
if (virSCSIDeviceIsAvailable(dev)) goto cleanup;
- if (virSCSIDeviceSetUsedBy(dev, "fc20") < 0) + if (virSCSIDeviceSetUsedBy(dev, "QEMU", "fc20") < 0) goto cleanup;
if (virSCSIDeviceIsAvailable(dev)) @@ -117,7 +117,7 @@ test2(const void *data ATTRIBUTE_UNUSED) if (!virSCSIDeviceListFind(list, dev)) goto cleanup;
- virSCSIDeviceListDel(list, dev, "fc20"); + virSCSIDeviceListDel(list, dev, "QEMU", "fc20");
if (!virSCSIDeviceListFind(list, dev)) goto cleanup;
May be some tests with the "LXC" value couldn't harm.
-- Cedric