[libvirt] PATCH: 20/28: Thread safety for node device driver
Daniel P. Berrange
berrange at redhat.com
Mon Dec 1 00:05:40 UTC 2008
This patch makes the node device driver thread safe. Almost.
There is on outstanding problem in that the 'reload' method is implemented
by just calling shutdown/startup. This causes the mutex we're locking on
to be free'd and re-allocated. I'll fix this up later.
libvirt_sym.version.in | 1
node_device.c | 53 ++++++++++++++-
node_device.h | 4 +
node_device_conf.h | 3
node_device_devkit.c | 41 ++++++++----
node_device_hal.c | 166 +++++++++++++++++++++++++++++++++----------------
6 files changed, 199 insertions(+), 69 deletions(-)
Daniel
diff --git a/src/libvirt_sym.version.in b/src/libvirt_sym.version.in
--- a/src/libvirt_sym.version.in
+++ b/src/libvirt_sym.version.in
@@ -509,6 +509,7 @@ LIBVIRT_PRIVATE_ at VERSION@ {
virNodeDeviceDefFormat;
virNodeDeviceObjLock;
virNodeDeviceObjUnlock;
+ virNodeDeviceAssignDef;
/* qparams.h */
diff --git a/src/node_device.c b/src/node_device.c
--- a/src/node_device.c
+++ b/src/node_device.c
@@ -29,7 +29,7 @@
#include "virterror_internal.h"
#include "datatypes.h"
#include "memory.h"
-
+#include "logging.h"
#include "node_device_conf.h"
#include "node_device.h"
@@ -44,6 +44,17 @@ static int dev_has_cap(const virNodeDevi
return 0;
}
+
+void nodeDeviceLock(virDeviceMonitorStatePtr driver)
+{
+ DEBUG("LOCK node %p", driver);
+ pthread_mutex_lock(&driver->lock);
+}
+void nodeDeviceUnlock(virDeviceMonitorStatePtr driver)
+{
+ DEBUG("UNLOCK node %p", driver);
+ pthread_mutex_unlock(&driver->lock);
+}
static int nodeNumOfDevices(virConnectPtr conn,
const char *cap,
@@ -71,15 +82,24 @@ nodeListDevices(virConnectPtr conn,
int ndevs = 0;
unsigned int i;
- for (i = 0; i < driver->devs.count && ndevs < maxnames; i++)
+ nodeDeviceLock(driver);
+ for (i = 0; i < driver->devs.count && ndevs < maxnames; i++) {
+ virNodeDeviceObjLock(driver->devs.objs[i]);
if (cap == NULL ||
- dev_has_cap(driver->devs.objs[i], cap))
- if ((names[ndevs++] = strdup(driver->devs.objs[i]->def->name)) == NULL)
+ dev_has_cap(driver->devs.objs[i], cap)) {
+ if ((names[ndevs++] = strdup(driver->devs.objs[i]->def->name)) == NULL) {
+ virNodeDeviceObjUnlock(driver->devs.objs[i]);
goto failure;
+ }
+ }
+ virNodeDeviceObjUnlock(driver->devs.objs[i]);
+ }
+ nodeDeviceUnlock(driver);
return ndevs;
failure:
+ nodeDeviceUnlock(driver);
--ndevs;
while (--ndevs >= 0)
VIR_FREE(names[ndevs]);
@@ -94,7 +114,10 @@ static virNodeDevicePtr nodeDeviceLookup
virNodeDeviceObjPtr obj;
virNodeDevicePtr ret = NULL;
+ nodeDeviceLock(driver);
obj = virNodeDeviceFindByName(&driver->devs, name);
+ nodeDeviceUnlock(driver);
+
if (!obj) {
virNodeDeviceReportError(conn, VIR_ERR_INVALID_NODE_DEVICE,
"%s", _("no node device with matching name"));
@@ -104,6 +127,8 @@ static virNodeDevicePtr nodeDeviceLookup
ret = virGetNodeDevice(conn, name);
cleanup:
+ if (obj)
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -114,7 +139,10 @@ static char *nodeDeviceDumpXML(virNodeDe
virNodeDeviceObjPtr obj;
char *ret = NULL;
+ nodeDeviceLock(driver);
obj = virNodeDeviceFindByName(&driver->devs, dev->name);
+ nodeDeviceUnlock(driver);
+
if (!obj) {
virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE,
"%s", _("no node device with matching name"));
@@ -124,6 +152,8 @@ static char *nodeDeviceDumpXML(virNodeDe
ret = virNodeDeviceDefFormat(dev->conn, obj->def);
cleanup:
+ if (obj)
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -134,7 +164,10 @@ static char *nodeDeviceGetParent(virNode
virNodeDeviceObjPtr obj;
char *ret = NULL;
+ nodeDeviceLock(driver);
obj = virNodeDeviceFindByName(&driver->devs, dev->name);
+ nodeDeviceUnlock(driver);
+
if (!obj) {
virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE,
"%s", _("no node device with matching name"));
@@ -144,6 +177,8 @@ static char *nodeDeviceGetParent(virNode
ret = strdup(obj->def->parent);
cleanup:
+ if (obj)
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -156,7 +191,10 @@ static int nodeDeviceNumOfCaps(virNodeDe
int ncaps = 0;
int ret = -1;
+ nodeDeviceLock(driver);
obj = virNodeDeviceFindByName(&driver->devs, dev->name);
+ nodeDeviceUnlock(driver);
+
if (!obj) {
virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE,
"%s", _("no node device with matching name"));
@@ -168,6 +206,8 @@ static int nodeDeviceNumOfCaps(virNodeDe
ret = ncaps;
cleanup:
+ if (obj)
+ virNodeDeviceObjUnlock(obj);
return ret;
}
@@ -181,7 +221,10 @@ nodeDeviceListCaps(virNodeDevicePtr dev,
int ncaps = 0;
int ret = -1;
+ nodeDeviceLock(driver);
obj = virNodeDeviceFindByName(&driver->devs, dev->name);
+ nodeDeviceUnlock(driver);
+
if (!obj) {
virNodeDeviceReportError(dev->conn, VIR_ERR_INVALID_NODE_DEVICE,
"%s", _("no node device with matching name"));
@@ -196,6 +239,8 @@ nodeDeviceListCaps(virNodeDevicePtr dev,
ret = ncaps;
cleanup:
+ if (obj)
+ virNodeDeviceObjUnlock(obj);
if (ret == -1) {
--ncaps;
while (--ncaps >= 0)
diff --git a/src/node_device.h b/src/node_device.h
--- a/src/node_device.h
+++ b/src/node_device.h
@@ -26,6 +26,7 @@
#include "internal.h"
#include "driver.h"
+#include "node_device_conf.h"
#ifdef HAVE_HAL
int halNodeRegister(void);
@@ -34,6 +35,9 @@ int devkitNodeRegister(void);
int devkitNodeRegister(void);
#endif
+void nodeDeviceLock(virDeviceMonitorStatePtr driver);
+void nodeDeviceUnlock(virDeviceMonitorStatePtr driver);
+
void registerCommonNodeFuncs(virDeviceMonitorPtr mon);
int nodedevRegister(void);
diff --git a/src/node_device_conf.h b/src/node_device_conf.h
--- a/src/node_device_conf.h
+++ b/src/node_device_conf.h
@@ -158,7 +158,8 @@ typedef struct _virDeviceMonitorState vi
typedef struct _virDeviceMonitorState virDeviceMonitorState;
typedef virDeviceMonitorState *virDeviceMonitorStatePtr;
struct _virDeviceMonitorState {
- int dbusWatch;
+ PTHREAD_MUTEX_T(lock);
+
virNodeDeviceObjList devs; /* currently-known devices */
void *privateData; /* driver-specific private data */
};
diff --git a/src/node_device_devkit.c b/src/node_device_devkit.c
--- a/src/node_device_devkit.c
+++ b/src/node_device_devkit.c
@@ -237,6 +237,7 @@ static void dev_create(void *_dkdev, voi
DevkitDevice *dkdev = _dkdev;
const char *sysfs_path = devkit_device_get_native_path(dkdev);
virNodeDeviceObjPtr dev = NULL;
+ virNodeDeviceDefPtr def = NULL;
const char *name;
int rv;
@@ -250,31 +251,39 @@ static void dev_create(void *_dkdev, voi
else
++name;
- if (VIR_ALLOC(dev) < 0 || VIR_ALLOC(dev->def) < 0)
+ if (VIR_ALLOC(def) < 0)
goto failure;
- dev->privateData = dkdev;
-
- if ((dev->def->name = strdup(name)) == NULL)
+ if ((def->name = strdup(name)) == NULL)
goto failure;
// TODO: Find device parent, if any
- rv = gather_capabilities(dkdev, &dev->def->caps);
+ rv = gather_capabilities(dkdev, &def->caps);
if (rv != 0) goto failure;
- if (VIR_REALLOC_N(driverState->devs.objs, driverState->devs.count + 1) < 0)
+ nodeDeviceLock(driverState);
+ dev = virNodeDeviceAssignDef(NULL,
+ &driverState->devs,
+ def);
+
+ if (!dev) {
+ nodeDeviceUnlock(driverState);
goto failure;
+ }
- driverState->devs.objs[driverState->devs.count++] = dev;
+ dev->privateData = dkdev;
+ dev->privateFree = NULL; /* XXX some free func needed ? */
+ virNodeDeviceObjUnlock(dev);
+
+ nodeDeviceUnlock(driverState);
return;
failure:
DEBUG("FAILED TO ADD dev %s", name);
- if (dev)
- virNodeDeviceDefFree(dev->def);
- VIR_FREE(dev);
+ if (def)
+ virNodeDeviceDefFree(def);
}
@@ -292,8 +301,8 @@ static int devkitDeviceMonitorStartup(vo
if (VIR_ALLOC(driverState) < 0)
return -1;
- // TODO: Is it really ok to call this multiple times??
- // Is there something analogous to call on close?
+ pthread_mutex_init(&driverState->lock, NULL);
+
g_type_init();
/* Get new devkit_client and connect to daemon */
@@ -362,10 +371,14 @@ static int devkitDeviceMonitorShutdown(v
static int devkitDeviceMonitorShutdown(void)
{
if (driverState) {
- DevkitClient *devkit_client = DRV_STATE_DKCLIENT(driverState);
+ DevkitClient *devkit_client;
+
+ nodeDeviceLock(driverState);
+ devkit_client = DRV_STATE_DKCLIENT(driverState);
virNodeDeviceObjListFree(&driverState->devs);
if (devkit_client)
g_object_unref(devkit_client);
+ nodeDeviceLock(driverState);
VIR_FREE(driverState);
return 0;
}
@@ -375,6 +388,8 @@ static int devkitDeviceMonitorShutdown(v
static int devkitDeviceMonitorReload(void)
{
+ /* XXX This isn't thread safe because its free'ing the thing
+ * we're locking */
(void)devkitDeviceMonitorShutdown();
return devkitDeviceMonitorStartup();
}
diff --git a/src/node_device_hal.c b/src/node_device_hal.c
--- a/src/node_device_hal.c
+++ b/src/node_device_hal.c
@@ -403,53 +403,62 @@ static void free_udi(void *udi)
VIR_FREE(udi);
}
-static void dev_create(char *udi)
+static void dev_create(const char *udi)
{
- LibHalContext *ctx = DRV_STATE_HAL_CTX(driverState);
+ LibHalContext *ctx;
char *parent_key = NULL;
- virNodeDeviceObjPtr dev;
+ virNodeDeviceObjPtr dev = NULL;
+ virNodeDeviceDefPtr def = NULL;
const char *name = hal_name(udi);
int rv;
+ char *privData = strdup(udi);
- if (VIR_ALLOC(dev) < 0 || VIR_ALLOC(dev->def) < 0)
+ if (!privData)
+ return;
+
+ nodeDeviceLock(driverState);
+ ctx = DRV_STATE_HAL_CTX(driverState);
+
+ if (VIR_ALLOC(def) < 0)
goto failure;
- dev->privateData = udi;
- dev->privateFree = free_udi;
-
- if ((dev->def->name = strdup(name)) == NULL)
+ if ((def->name = strdup(name)) == NULL)
goto failure;
if (get_str_prop(ctx, udi, "info.parent", &parent_key) == 0) {
- dev->def->parent = strdup(hal_name(parent_key));
+ def->parent = strdup(hal_name(parent_key));
VIR_FREE(parent_key);
- if (dev->def->parent == NULL)
+ if (def->parent == NULL)
goto failure;
}
- rv = gather_capabilities(ctx, udi, &dev->def->caps);
+ rv = gather_capabilities(ctx, udi, &def->caps);
if (rv != 0) goto failure;
- if (dev->def->caps == NULL) {
- virNodeDeviceDefFree(dev->def);
- VIR_FREE(dev);
- VIR_FREE(udi);
- return;
- }
+ if (def->caps == NULL)
+ goto cleanup;
- if (VIR_REALLOC_N(driverState->devs.objs, driverState->devs.count + 1) < 0)
+ dev = virNodeDeviceAssignDef(NULL,
+ &driverState->devs,
+ def);
+
+ if (!dev)
goto failure;
- driverState->devs.objs[driverState->devs.count++] = dev;
+ dev->privateData = privData;
+ dev->privateFree = free_udi;
+ virNodeDeviceObjUnlock(dev);
+ nodeDeviceUnlock(driverState);
return;
failure:
DEBUG("FAILED TO ADD dev %s", name);
- if (dev)
- virNodeDeviceDefFree(dev->def);
- VIR_FREE(dev);
- VIR_FREE(udi);
+cleanup:
+ VIR_FREE(privData);
+ if (def)
+ virNodeDeviceDefFree(def);
+ nodeDeviceUnlock(driverState);
}
@@ -457,7 +466,7 @@ static void device_added(LibHalContext *
const char *udi)
{
DEBUG0(hal_name(udi));
- dev_create(strdup(udi));
+ dev_create(udi);
}
@@ -465,12 +474,16 @@ static void device_removed(LibHalContext
const char *udi)
{
const char *name = hal_name(udi);
- virNodeDeviceObjPtr dev = virNodeDeviceFindByName(&driverState->devs,name);
+ virNodeDeviceObjPtr dev;
+
+ nodeDeviceLock(driverState);
+ dev = virNodeDeviceFindByName(&driverState->devs,name);
DEBUG0(name);
if (dev)
virNodeDeviceObjRemove(&driverState->devs, dev);
else
DEBUG("no device named %s", name);
+ nodeDeviceUnlock(driverState);
}
@@ -478,12 +491,18 @@ static void device_cap_added(LibHalConte
const char *udi, const char *cap)
{
const char *name = hal_name(udi);
- virNodeDeviceObjPtr dev = virNodeDeviceFindByName(&driverState->devs,name);
+ virNodeDeviceObjPtr dev;
+
+ nodeDeviceLock(driverState);
+ dev = virNodeDeviceFindByName(&driverState->devs,name);
+ nodeDeviceUnlock(driverState);
DEBUG("%s %s", cap, name);
- if (dev)
+ if (dev) {
(void)gather_capability(ctx, udi, cap, &dev->def->caps);
- else
+ virNodeDeviceObjUnlock(dev);
+ } else {
DEBUG("no device named %s", name);
+ }
}
@@ -492,16 +511,20 @@ static void device_cap_lost(LibHalContex
const char *cap)
{
const char *name = hal_name(udi);
- virNodeDeviceObjPtr dev = virNodeDeviceFindByName(&driverState->devs,name);
+ virNodeDeviceObjPtr dev;
+
+ nodeDeviceLock(driverState);
+ dev = virNodeDeviceFindByName(&driverState->devs,name);
DEBUG("%s %s", cap, name);
if (dev) {
/* Simply "rediscover" device -- incrementally handling changes
* to sub-capabilities (like net.80203) is nasty ... so avoid it.
*/
virNodeDeviceObjRemove(&driverState->devs, dev);
- dev_create(strdup(udi));
+ dev_create(udi);
} else
DEBUG("no device named %s", name);
+ nodeDeviceUnlock(driverState);
}
@@ -512,7 +535,10 @@ static void device_prop_modified(LibHalC
dbus_bool_t is_added ATTRIBUTE_UNUSED)
{
const char *name = hal_name(udi);
- virNodeDeviceObjPtr dev = virNodeDeviceFindByName(&driverState->devs,name);
+ virNodeDeviceObjPtr dev;
+
+ nodeDeviceLock(driverState);
+ dev = virNodeDeviceFindByName(&driverState->devs,name);
DEBUG("%s %s", key, name);
if (dev) {
/* Simply "rediscover" device -- incrementally handling changes
@@ -520,9 +546,10 @@ static void device_prop_modified(LibHalC
* specific ways) is nasty ... so avoid it.
*/
virNodeDeviceObjRemove(&driverState->devs, dev);
- dev_create(strdup(udi));
+ dev_create(udi);
} else
DEBUG("no device named %s", name);
+ nodeDeviceUnlock(driverState);
}
@@ -531,8 +558,8 @@ static void dbus_watch_callback(int fdat
int events, void *opaque)
{
DBusWatch *watch = opaque;
- LibHalContext *hal_ctx = DRV_STATE_HAL_CTX(driverState);
- DBusConnection *dbus_conn = libhal_ctx_get_dbus_connection(hal_ctx);
+ LibHalContext *hal_ctx;
+ DBusConnection *dbus_conn;
int dbus_flags = 0;
if (events & VIR_EVENT_HANDLE_READABLE)
@@ -546,6 +573,10 @@ static void dbus_watch_callback(int fdat
(void)dbus_watch_handle(watch, dbus_flags);
+ nodeDeviceLock(driverState);
+ hal_ctx = DRV_STATE_HAL_CTX(driverState);
+ dbus_conn = libhal_ctx_get_dbus_connection(hal_ctx);
+ nodeDeviceUnlock(driverState);
while (dbus_connection_dispatch(dbus_conn) == DBUS_DISPATCH_DATA_REMAINS)
/* keep dispatching while data remains */;
}
@@ -566,42 +597,63 @@ static int xlate_dbus_watch_flags(int db
}
+struct nodeDeviceWatchInfo
+{
+ int watch;
+};
+
+static void nodeDeviceWatchFree(void *data) {
+ struct nodeDeviceWatchInfo *info = data;
+ VIR_FREE(info);
+}
+
static dbus_bool_t add_dbus_watch(DBusWatch *watch,
- void *data)
+ void *data ATTRIBUTE_UNUSED)
{
int flags = 0;
- virDeviceMonitorStatePtr state = data;
+ struct nodeDeviceWatchInfo *info;
+
+ if (VIR_ALLOC(info) < 0)
+ return 0;
if (dbus_watch_get_enabled(watch))
flags = xlate_dbus_watch_flags(dbus_watch_get_flags(watch));
- if ((state->dbusWatch =
- virEventAddHandle(dbus_watch_get_unix_fd(watch), flags,
- dbus_watch_callback, watch, NULL)) < 0)
+ info->watch = virEventAddHandle(dbus_watch_get_unix_fd(watch), flags,
+ dbus_watch_callback, watch, NULL);
+ if (info->watch < 0) {
+ VIR_FREE(info);
return 0;
+ }
+ dbus_watch_set_data(watch, info, nodeDeviceWatchFree);
+
return 1;
}
-static void remove_dbus_watch(DBusWatch *watch ATTRIBUTE_UNUSED,
- void *data)
+static void remove_dbus_watch(DBusWatch *watch,
+ void *data ATTRIBUTE_UNUSED)
{
- virDeviceMonitorStatePtr state = data;
+ struct nodeDeviceWatchInfo *info;
- (void)virEventRemoveHandle(state->dbusWatch);
+ info = dbus_watch_get_data(watch);
+
+ (void)virEventRemoveHandle(info->watch);
}
static void toggle_dbus_watch(DBusWatch *watch,
- void *data)
+ void *data ATTRIBUTE_UNUSED)
{
int flags = 0;
- virDeviceMonitorStatePtr state = data;
+ struct nodeDeviceWatchInfo *info;
if (dbus_watch_get_enabled(watch))
flags = xlate_dbus_watch_flags(dbus_watch_get_flags(watch));
- (void)virEventUpdateHandle(state->dbusWatch, flags);
+ info = dbus_watch_get_data(watch);
+
+ (void)virEventUpdateHandle(info->watch, flags);
}
@@ -619,6 +671,9 @@ static int halDeviceMonitorStartup(void)
if (VIR_ALLOC(driverState) < 0)
return -1;
+
+ pthread_mutex_init(&driverState->lock, NULL);
+ nodeDeviceLock(driverState);
/* Allocate and initialize a new HAL context */
dbus_error_init(&err);
@@ -647,7 +702,7 @@ static int halDeviceMonitorStartup(void)
add_dbus_watch,
remove_dbus_watch,
toggle_dbus_watch,
- driverState, NULL)) {
+ NULL, NULL)) {
fprintf(stderr, "%s: dbus_connection_set_watch_functions failed\n",
__FUNCTION__);
goto failure;
@@ -665,14 +720,18 @@ static int halDeviceMonitorStartup(void)
/* Populate with known devices */
driverState->privateData = hal_ctx;
+
+ nodeDeviceUnlock(driverState);
udi = libhal_get_all_devices(hal_ctx, &num_devs, &err);
if (udi == NULL) {
fprintf(stderr, "%s: libhal_get_all_devices failed\n", __FUNCTION__);
goto failure;
}
- for (i = 0; i < num_devs; i++)
+ for (i = 0; i < num_devs; i++) {
dev_create(udi[i]);
- free(udi);
+ VIR_FREE(udi[i]);
+ }
+ VIR_FREE(udi);
return 0;
@@ -686,9 +745,10 @@ static int halDeviceMonitorStartup(void)
(void)libhal_ctx_free(hal_ctx);
if (udi) {
for (i = 0; i < num_devs; i++)
- free(udi[i]);
- free(udi);
+ VIR_FREE(udi[i]);
+ VIR_FREE(udi);
}
+ nodeDeviceUnlock(driverState);
VIR_FREE(driverState);
return -1;
@@ -698,10 +758,12 @@ static int halDeviceMonitorShutdown(void
static int halDeviceMonitorShutdown(void)
{
if (driverState) {
+ nodeDeviceLock(driverState);
LibHalContext *hal_ctx = DRV_STATE_HAL_CTX(driverState);
virNodeDeviceObjListFree(&driverState->devs);
(void)libhal_ctx_shutdown(hal_ctx, NULL);
(void)libhal_ctx_free(hal_ctx);
+ nodeDeviceUnlock(driverState);
VIR_FREE(driverState);
return 0;
}
@@ -711,6 +773,8 @@ static int halDeviceMonitorShutdown(void
static int halDeviceMonitorReload(void)
{
+ /* XXX This isn't thread safe because its free'ing the thing
+ * we're locking */
(void)halDeviceMonitorShutdown();
return halDeviceMonitorStartup();
}
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
More information about the libvir-list
mailing list