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_@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(); }