[libvirt PATCH 00/12] Automatic mutex management - part 4
Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD to simplify mutex management. Tim Wiederhake (12): nwfilter_driver: Statically initialize mutex nwfilter_driver: Split up nwfilterStateCleanup nwfilter_driver: Use automatic mutex management nwfilter_gentech: Use automatic mutex management nwfilter_dhcpsnoop: Replace virNWFilterSnoopActiveLock macros nwfilter_dhcpsnoop: Replace virNWFilterSnoopLock macros nwfilter_dhcpsnoop: Replace virNWFilterSnoopReqLock functions nwfilter_learnipaddr: Use automatic mutex management nwfilter_ipaddrmap: Use automatic mutex management virnetlink: Use automatic memory management remote_daemon_stream: Use automatic memory management qemu_conf: Use automatic memory management src/conf/nwfilter_ipaddrmap.c | 80 ++--- src/conf/virnwfilterobj.h | 1 - src/nwfilter/nwfilter_dhcpsnoop.c | 393 ++++++++----------------- src/nwfilter/nwfilter_driver.c | 175 +++++------ src/nwfilter/nwfilter_gentech_driver.c | 34 +-- src/nwfilter/nwfilter_learnipaddr.c | 83 ++---- src/qemu/qemu_conf.c | 70 ++--- src/remote/remote_daemon_stream.c | 34 +-- src/util/virnetlink.c | 101 +++---- 9 files changed, 357 insertions(+), 614 deletions(-) -- 2.31.1
This enables a later patch to simplify locking during initialization and cleanup of virNWFilterDriverState. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virnwfilterobj.h | 1 - src/nwfilter/nwfilter_driver.c | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 44ba31f732..c365d0f28a 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -30,7 +30,6 @@ typedef struct _virNWFilterObjList virNWFilterObjList; typedef struct _virNWFilterDriverState virNWFilterDriverState; struct _virNWFilterDriverState { - virMutex lock; bool privileged; /* pid file FD, ensures two copies of the driver can't use the same root */ diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 3ce8fce7f9..26edfd3691 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -57,13 +57,15 @@ static int nwfilterStateCleanup(void); static int nwfilterStateReload(void); +static virMutex mutex = VIR_MUTEX_INITIALIZER; + static void nwfilterDriverLock(void) { - virMutexLock(&driver->lock); + virMutexLock(&mutex); } static void nwfilterDriverUnlock(void) { - virMutexUnlock(&driver->lock); + virMutexUnlock(&mutex); } #ifdef WITH_FIREWALLD @@ -174,10 +176,8 @@ nwfilterStateInitialize(bool privileged, driver = g_new0(virNWFilterDriverState, 1); driver->lockFD = -1; - if (virMutexInit(&driver->lock) < 0) - goto err_free_driverstate; - driver->privileged = privileged; + if (!(driver->nwfilters = virNWFilterObjListNew())) goto error; @@ -343,7 +343,6 @@ nwfilterStateCleanup(void) /* free inactive nwfilters */ virNWFilterObjListFree(driver->nwfilters); - virMutexDestroy(&driver->lock); g_clear_pointer(&driver, g_free); return 0; -- 2.31.1
On Wed, Mar 09, 2022 at 12:02:19PM +0100, Tim Wiederhake wrote:
This enables a later patch to simplify locking during initialization and cleanup of virNWFilterDriverState.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virnwfilterobj.h | 1 - src/nwfilter/nwfilter_driver.c | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 44ba31f732..c365d0f28a 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -30,7 +30,6 @@ typedef struct _virNWFilterObjList virNWFilterObjList;
typedef struct _virNWFilterDriverState virNWFilterDriverState; struct _virNWFilterDriverState { - virMutex lock; bool privileged;
/* pid file FD, ensures two copies of the driver can't use the same root */ diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 3ce8fce7f9..26edfd3691 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -57,13 +57,15 @@ static int nwfilterStateCleanup(void);
static int nwfilterStateReload(void);
+static virMutex mutex = VIR_MUTEX_INITIALIZER;
Please call this 'driverLock'. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This allows nwfilterStateCleanupLocked to be used in nwfilterStateInitialize in a later patch. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_driver.c | 89 +++++++++++++++++----------------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 26edfd3691..8eea9e5805 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -53,8 +53,6 @@ VIR_LOG_INIT("nwfilter.nwfilter_driver"); static virNWFilterDriverState *driver; -static int nwfilterStateCleanup(void); - static int nwfilterStateReload(void); static virMutex mutex = VIR_MUTEX_INITIALIZER; @@ -150,6 +148,51 @@ virNWFilterTriggerRebuildImpl(void *opaque) } +static int +nwfilterStateCleanupLocked(void) +{ + if (!driver) + return -1; + + if (driver->privileged) { + virNWFilterConfLayerShutdown(); + virNWFilterDHCPSnoopShutdown(); + virNWFilterLearnShutdown(); + virNWFilterIPAddrMapShutdown(); + virNWFilterTechDriversShutdown(); + nwfilterDriverRemoveDBusMatches(); + + if (driver->lockFD != -1) + virPidFileRelease(driver->stateDir, "driver", driver->lockFD); + + g_free(driver->stateDir); + g_free(driver->configDir); + g_free(driver->bindingDir); + } + + virObjectUnref(driver->bindings); + + /* free inactive nwfilters */ + virNWFilterObjListFree(driver->nwfilters); + + g_clear_pointer(&driver, g_free); + + return 0; +} + +/** + * nwfilterStateCleanup: + * + * Shutdown the nwfilter driver, it will stop all active nwfilters + */ +static int +nwfilterStateCleanup(void) +{ + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + return nwfilterStateCleanupLocked(); +} + + /** * nwfilterStateInitialize: * @@ -307,48 +350,6 @@ nwfilterStateReload(void) } -/** - * nwfilterStateCleanup: - * - * Shutdown the nwfilter driver, it will stop all active nwfilters - */ -static int -nwfilterStateCleanup(void) -{ - if (!driver) - return -1; - - if (driver->privileged) { - virNWFilterConfLayerShutdown(); - virNWFilterDHCPSnoopShutdown(); - virNWFilterLearnShutdown(); - virNWFilterIPAddrMapShutdown(); - virNWFilterTechDriversShutdown(); - - nwfilterDriverLock(); - - nwfilterDriverRemoveDBusMatches(); - - if (driver->lockFD != -1) - virPidFileRelease(driver->stateDir, "driver", driver->lockFD); - - g_free(driver->stateDir); - g_free(driver->configDir); - g_free(driver->bindingDir); - nwfilterDriverUnlock(); - } - - virObjectUnref(driver->bindings); - - /* free inactive nwfilters */ - virNWFilterObjListFree(driver->nwfilters); - - g_clear_pointer(&driver, g_free); - - return 0; -} - - static virDrvOpenStatus nwfilterConnectOpen(virConnectPtr conn, virConnectAuthPtr auth G_GNUC_UNUSED, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_driver.c | 83 +++++++++++++--------------------- 1 file changed, 32 insertions(+), 51 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 8eea9e5805..12bbbc661f 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -57,15 +57,6 @@ static int nwfilterStateReload(void); static virMutex mutex = VIR_MUTEX_INITIALIZER; -static void nwfilterDriverLock(void) -{ - virMutexLock(&mutex); -} -static void nwfilterDriverUnlock(void) -{ - virMutexUnlock(&mutex); -} - #ifdef WITH_FIREWALLD static void @@ -204,6 +195,7 @@ nwfilterStateInitialize(bool privileged, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); GDBusConnection *sysbus = NULL; if (root != NULL) { @@ -230,8 +222,6 @@ nwfilterStateInitialize(bool privileged, if (!privileged) return VIR_DRV_STATE_INIT_SKIPPED; - nwfilterDriverLock(); - driver->stateDir = g_strdup(RUNSTATEDIR "/libvirt/nwfilter"); if (g_mkdir_with_parents(driver->stateDir, S_IRWXU) < 0) { @@ -290,13 +280,10 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterBuildAll(driver, false) < 0) goto error; - nwfilterDriverUnlock(); - return VIR_DRV_STATE_INIT_COMPLETE; error: - nwfilterDriverUnlock(); - nwfilterStateCleanup(); + nwfilterStateCleanupLocked(); return VIR_DRV_STATE_INIT_ERROR; @@ -335,16 +322,15 @@ nwfilterStateReload(void) /* shut down all threads -- they will be restarted if necessary */ virNWFilterLearnThreadsTerminate(true); - nwfilterDriverLock(); - virNWFilterWriteLockFilterUpdates(); - - virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + virNWFilterWriteLockFilterUpdates(); - virNWFilterUnlockFilterUpdates(); + virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); - virNWFilterBuildAll(driver, false); + virNWFilterUnlockFilterUpdates(); - nwfilterDriverUnlock(); + virNWFilterBuildAll(driver, false); + } return 0; } @@ -422,13 +408,13 @@ static virNWFilterPtr nwfilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - virNWFilterObj *obj; + virNWFilterObj *obj = NULL; virNWFilterDef *def; virNWFilterPtr nwfilter = NULL; - nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(uuid); - nwfilterDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + obj = nwfilterObjFromNWFilter(uuid); + } if (!obj) return NULL; @@ -449,13 +435,13 @@ static virNWFilterPtr nwfilterLookupByName(virConnectPtr conn, const char *name) { - virNWFilterObj *obj; + virNWFilterObj *obj = NULL; virNWFilterDef *def; virNWFilterPtr nwfilter = NULL; - nwfilterDriverLock(); - obj = virNWFilterObjListFindByName(driver->nwfilters, name); - nwfilterDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + obj = virNWFilterObjListFindByName(driver->nwfilters, name); + } if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, @@ -491,17 +477,16 @@ nwfilterConnectListNWFilters(virConnectPtr conn, char **const names, int maxnames) { - int nnames; + VIR_LOCK_GUARD lock = { NULL }; if (virConnectListNWFiltersEnsureACL(conn) < 0) return -1; - nwfilterDriverLock(); - nnames = virNWFilterObjListGetNames(driver->nwfilters, conn, - virConnectListNWFiltersCheckACL, - names, maxnames); - nwfilterDriverUnlock(); - return nnames; + lock = virLockGuardLock(&mutex); + + return virNWFilterObjListGetNames(driver->nwfilters, conn, + virConnectListNWFiltersCheckACL, + names, maxnames); } @@ -510,19 +495,17 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn, virNWFilterPtr **nwfilters, unsigned int flags) { - int ret; + VIR_LOCK_GUARD lock = { NULL }; virCheckFlags(0, -1); if (virConnectListAllNWFiltersEnsureACL(conn) < 0) return -1; - nwfilterDriverLock(); - ret = virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, - virConnectListAllNWFiltersCheckACL); - nwfilterDriverUnlock(); + lock = virLockGuardLock(&mutex); - return ret; + return virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, + virConnectListAllNWFiltersCheckACL); } @@ -531,6 +514,7 @@ nwfilterDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); virNWFilterDef *def; virNWFilterObj *obj = NULL; virNWFilterDef *objdef; @@ -545,7 +529,6 @@ nwfilterDefineXMLFlags(virConnectPtr conn, return NULL; } - nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); if (!(def = virNWFilterDefParseString(xml, flags))) @@ -572,7 +555,6 @@ nwfilterDefineXMLFlags(virConnectPtr conn, virNWFilterObjUnlock(obj); virNWFilterUnlockFilterUpdates(); - nwfilterDriverUnlock(); return nwfilter; } @@ -588,11 +570,11 @@ nwfilterDefineXML(virConnectPtr conn, static int nwfilterUndefine(virNWFilterPtr nwfilter) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); virNWFilterObj *obj; virNWFilterDef *def; int ret = -1; - nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) @@ -621,7 +603,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterObjUnlock(obj); virNWFilterUnlockFilterUpdates(); - nwfilterDriverUnlock(); return ret; } @@ -630,15 +611,15 @@ static char * nwfilterGetXMLDesc(virNWFilterPtr nwfilter, unsigned int flags) { - virNWFilterObj *obj; + virNWFilterObj *obj = NULL; virNWFilterDef *def; char *ret = NULL; virCheckFlags(0, NULL); - nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(nwfilter->uuid); - nwfilterDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + obj = nwfilterObjFromNWFilter(nwfilter->uuid); + } if (!obj) return NULL; -- 2.31.1
On Wed, Mar 09, 2022 at 12:02:21PM +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_driver.c | 83 +++++++++++++--------------------- 1 file changed, 32 insertions(+), 51 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 8eea9e5805..12bbbc661f 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -57,15 +57,6 @@ static int nwfilterStateReload(void);
static virMutex mutex = VIR_MUTEX_INITIALIZER;
-static void nwfilterDriverLock(void) -{ - virMutexLock(&mutex); -} -static void nwfilterDriverUnlock(void) -{ - virMutexUnlock(&mutex); -} - #ifdef WITH_FIREWALLD
static void @@ -204,6 +195,7 @@ nwfilterStateInitialize(bool privileged, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); GDBusConnection *sysbus = NULL;
if (root != NULL) { @@ -230,8 +222,6 @@ nwfilterStateInitialize(bool privileged, if (!privileged) return VIR_DRV_STATE_INIT_SKIPPED;
- nwfilterDriverLock(); - driver->stateDir = g_strdup(RUNSTATEDIR "/libvirt/nwfilter");
if (g_mkdir_with_parents(driver->stateDir, S_IRWXU) < 0) { @@ -290,13 +280,10 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterBuildAll(driver, false) < 0) goto error;
- nwfilterDriverUnlock(); - return VIR_DRV_STATE_INIT_COMPLETE;
error: - nwfilterDriverUnlock(); - nwfilterStateCleanup(); + nwfilterStateCleanupLocked();
return VIR_DRV_STATE_INIT_ERROR;
@@ -335,16 +322,15 @@ nwfilterStateReload(void) /* shut down all threads -- they will be restarted if necessary */ virNWFilterLearnThreadsTerminate(true);
- nwfilterDriverLock(); - virNWFilterWriteLockFilterUpdates(); - - virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + virNWFilterWriteLockFilterUpdates();
- virNWFilterUnlockFilterUpdates(); + virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir);
- virNWFilterBuildAll(driver, false); + virNWFilterUnlockFilterUpdates();
- nwfilterDriverUnlock(); + virNWFilterBuildAll(driver, false); + }
return 0; } @@ -422,13 +408,13 @@ static virNWFilterPtr nwfilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - virNWFilterObj *obj; + virNWFilterObj *obj = NULL; virNWFilterDef *def; virNWFilterPtr nwfilter = NULL;
- nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(uuid); - nwfilterDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + obj = nwfilterObjFromNWFilter(uuid); + }
if (!obj) return NULL; @@ -449,13 +435,13 @@ static virNWFilterPtr nwfilterLookupByName(virConnectPtr conn, const char *name) { - virNWFilterObj *obj; + virNWFilterObj *obj = NULL; virNWFilterDef *def; virNWFilterPtr nwfilter = NULL;
- nwfilterDriverLock(); - obj = virNWFilterObjListFindByName(driver->nwfilters, name); - nwfilterDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + obj = virNWFilterObjListFindByName(driver->nwfilters, name); + }
if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, @@ -491,17 +477,16 @@ nwfilterConnectListNWFilters(virConnectPtr conn, char **const names, int maxnames) { - int nnames; + VIR_LOCK_GUARD lock = { NULL };
This is introducing a 3rd locking pattern, not used in any other conversions until now, which feels undesirable to me...
if (virConnectListNWFiltersEnsureACL(conn) < 0) return -1;
- nwfilterDriverLock(); - nnames = virNWFilterObjListGetNames(driver->nwfilters, conn, - virConnectListNWFiltersCheckACL, - names, maxnames); - nwfilterDriverUnlock(); - return nnames; + lock = virLockGuardLock(&mutex); + + return virNWFilterObjListGetNames(driver->nwfilters, conn, + virConnectListNWFiltersCheckACL, + names, maxnames);
...why isn't this just using VIR_WITH_MUTEX_LOCK_GUARD if we don't want the critical section to cover the entire method. Same point for any other case of the same pattern through this series. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 34 +++++++++----------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 7bbf1e12fb..3f888ce873 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -717,9 +717,7 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverState *driver, bool *foundNewFilter) { int ifindex; - int rc; - - virMutexLock(&updateMutex); + VIR_LOCK_GUARD lock = virLockGuardLock(&updateMutex); /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed @@ -729,20 +727,14 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverState *driver, /* interfaces / VMs can disappear during filter instantiation; don't mark it as an error */ virResetLastError(); - rc = 0; - goto cleanup; + return 0; } - rc = virNWFilterInstantiateFilterUpdate(driver, teardownOld, - binding, - ifindex, - useNewFilter, - false, foundNewFilter); - - cleanup: - virMutexUnlock(&updateMutex); - - return rc; + return virNWFilterInstantiateFilterUpdate(driver, teardownOld, + binding, + ifindex, + useNewFilter, + false, foundNewFilter); } @@ -753,9 +745,10 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverState *driver, { int rc; bool foundNewFilter = false; + VIR_LOCK_GUARD lock = { NULL }; virNWFilterReadLockFilterUpdates(); - virMutexLock(&updateMutex); + lock = virLockGuardLock(&updateMutex); rc = virNWFilterInstantiateFilterUpdate(driver, true, binding, ifindex, @@ -772,7 +765,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverState *driver, } virNWFilterUnlockFilterUpdates(); - virMutexUnlock(&updateMutex); return rc; } @@ -894,11 +886,9 @@ _virNWFilterTeardownFilter(const char *ifname) int virNWFilterTeardownFilter(virNWFilterBindingDef *binding) { - int ret; - virMutexLock(&updateMutex); - ret = _virNWFilterTeardownFilter(binding->portdevname); - virMutexUnlock(&updateMutex); - return ret; + VIR_LOCK_GUARD lock = virLockGuardLock(&updateMutex); + + return _virNWFilterTeardownFilter(binding->portdevname); } enum { -- 2.31.1
Use automatic mutex management instead. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 42 ++++++++----------------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index f8cffc7d57..c526653bc2 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -93,14 +93,6 @@ struct virNWFilterSnoopState { do { \ virMutexUnlock(&virNWFilterSnoopState.snoopLock); \ } while (0) -# define virNWFilterSnoopActiveLock() \ - do { \ - virMutexLock(&virNWFilterSnoopState.activeLock); \ - } while (0) -# define virNWFilterSnoopActiveUnlock() \ - do { \ - virMutexUnlock(&virNWFilterSnoopState.activeLock); \ - } while (0) # define VIR_IFKEY_LEN ((VIR_UUID_STRING_BUFLEN) + (VIR_MAC_STRING_BUFLEN)) @@ -281,47 +273,35 @@ static char * virNWFilterSnoopActivate(virNWFilterSnoopReq *req) { g_autofree char *key = g_strdup_printf("%p-%d", req, req->ifindex); - char *ret = NULL; + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.activeLock); - virNWFilterSnoopActiveLock(); - - if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) == 0) - ret = g_steal_pointer(&key); - - virNWFilterSnoopActiveUnlock(); + if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) < 0) + return NULL; - return ret; + return g_steal_pointer(&key); } static void virNWFilterSnoopCancel(char **threadKey) { + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.activeLock); + if (*threadKey == NULL) return; - virNWFilterSnoopActiveLock(); - ignore_value(virHashRemoveEntry(virNWFilterSnoopState.active, *threadKey)); g_clear_pointer(threadKey, g_free); - - virNWFilterSnoopActiveUnlock(); } static bool virNWFilterSnoopIsActive(char *threadKey) { - void *entry; + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.activeLock); if (threadKey == NULL) return false; - virNWFilterSnoopActiveLock(); - - entry = virHashLookup(virNWFilterSnoopState.active, threadKey); - - virNWFilterSnoopActiveUnlock(); - - return entry != NULL; + return virHashLookup(virNWFilterSnoopState.active, threadKey) != NULL; } /* @@ -2083,9 +2063,9 @@ virNWFilterDHCPSnoopShutdown(void) virNWFilterSnoopUnlock(); - virNWFilterSnoopActiveLock(); - g_clear_pointer(&virNWFilterSnoopState.active, g_hash_table_unref); - virNWFilterSnoopActiveUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.activeLock) { + g_clear_pointer(&virNWFilterSnoopState.active, g_hash_table_unref); + } } #else /* WITH_LIBPCAP */ -- 2.31.1
Use automatic mutex management instead. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 90 +++++++++++-------------------- 1 file changed, 30 insertions(+), 60 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index c526653bc2..f18f7b80d6 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -85,15 +85,6 @@ struct virNWFilterSnoopState { virMutex activeLock; /* protects Active */ }; -# define virNWFilterSnoopLock() \ - do { \ - virMutexLock(&virNWFilterSnoopState.snoopLock); \ - } while (0) -# define virNWFilterSnoopUnlock() \ - do { \ - virMutexUnlock(&virNWFilterSnoopState.snoopLock); \ - } while (0) - # define VIR_IFKEY_LEN ((VIR_UUID_STRING_BUFLEN) + (VIR_MAC_STRING_BUFLEN)) typedef struct _virNWFilterSnoopReq virNWFilterSnoopReq; @@ -147,7 +138,7 @@ struct _virNWFilterSnoopReq { /* * Note about lock-order: - * 1st: virNWFilterSnoopLock() + * 1st: virNWFilterSnoopState.snoopLock * 2nd: virNWFilterSnoopReqLock(req) * * Rationale: Former protects the SnoopReqs hash, latter its contents @@ -620,16 +611,13 @@ virNWFilterSnoopReqRelease(void *req0) static virNWFilterSnoopReq * virNWFilterSnoopReqGetByIFKey(const char *ifkey) { + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); virNWFilterSnoopReq *req; - virNWFilterSnoopLock(); - req = virHashLookup(virNWFilterSnoopState.snoopReqs, ifkey); if (req) virNWFilterSnoopReqGet(req); - virNWFilterSnoopUnlock(); - return req; } @@ -640,11 +628,11 @@ virNWFilterSnoopReqGetByIFKey(const char *ifkey) static void virNWFilterSnoopReqPut(virNWFilterSnoopReq *req) { + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); + if (!req) return; - virNWFilterSnoopLock(); - if (!!g_atomic_int_dec_and_test(&req->refctr)) { /* * delete the request: @@ -660,8 +648,6 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReq *req) req->ifkey)); } } - - virNWFilterSnoopUnlock(); } /* @@ -1460,20 +1446,19 @@ virNWFilterDHCPSnoopThread(void *req0) } /* while (!error) */ /* protect IfNameToKey */ - virNWFilterSnoopLock(); - - /* protect req->binding->portdevname & req->threadkey */ - virNWFilterSnoopReqLock(req); + VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.snoopLock) { + /* protect req->binding->portdevname & req->threadkey */ + virNWFilterSnoopReqLock(req); - virNWFilterSnoopCancel(&req->threadkey); + virNWFilterSnoopCancel(&req->threadkey); - ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, - req->binding->portdevname)); + ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, + req->binding->portdevname)); - g_clear_pointer(&req->binding->portdevname, g_free); + g_clear_pointer(&req->binding->portdevname, g_free); - virNWFilterSnoopReqUnlock(req); - virNWFilterSnoopUnlock(); + virNWFilterSnoopReqUnlock(req); + } cleanup: virThreadPoolFree(worker); @@ -1511,6 +1496,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, virThread thread; virNWFilterVarValue *dhcpsrvrs; bool threadPuts = false; + VIR_LOCK_GUARD lock = { NULL }; virNWFilterSnoopIFKeyFMT(ifkey, binding->owneruuid, &binding->mac); @@ -1556,7 +1542,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, goto exit_snoopreqput; } - virNWFilterSnoopLock(); + lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); if (virHashAddEntry(virNWFilterSnoopState.ifnameToKey, req->binding->portdevname, @@ -1621,8 +1607,6 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, virNWFilterSnoopReqUnlock(req); - virNWFilterSnoopUnlock(); - /* do not 'put' the req -- the thread will do this */ return 0; @@ -1634,7 +1618,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, exit_rem_ifnametokey: virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, binding->portdevname); exit_snoopunlock: - virNWFilterSnoopUnlock(); + virLockGuardUnlock(&lock); exit_snoopreqput: if (!threadPuts) virNWFilterSnoopReqPut(req); @@ -1695,23 +1679,19 @@ virNWFilterSnoopLeaseFileWrite(int lfd, const char *ifkey, static void virNWFilterSnoopLeaseFileSave(virNWFilterSnoopIPLease *ipl) { + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); virNWFilterSnoopReq *req = ipl->snoopReq; - virNWFilterSnoopLock(); - if (virNWFilterSnoopState.leaseFD < 0) virNWFilterSnoopLeaseFileOpen(); if (virNWFilterSnoopLeaseFileWrite(virNWFilterSnoopState.leaseFD, req->ifkey, ipl) < 0) - goto error; + return; /* keep dead leases at < ~95% of file size */ if (g_atomic_int_add(&virNWFilterSnoopState.wLeases, 1) >= g_atomic_int_get(&virNWFilterSnoopState.nLeases) * 20) virNWFilterSnoopLeaseFileLoad(); /* load & refresh lease file */ - - error: - virNWFilterSnoopUnlock(); } /* @@ -1830,9 +1810,7 @@ virNWFilterSnoopLeaseFileLoad(void) time_t now; FILE *fp; int ln = 0, tmp; - - /* protect the lease file */ - virNWFilterSnoopLock(); + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); fp = fopen(LEASEFILE, "r"); time(&now); @@ -1892,8 +1870,6 @@ virNWFilterSnoopLeaseFileLoad(void) VIR_FORCE_FCLOSE(fp); virNWFilterSnoopLeaseFileRefresh(); - - virNWFilterSnoopUnlock(); } /* @@ -1953,11 +1929,11 @@ virNWFilterSnoopRemAllReqIter(const void *payload, static void virNWFilterSnoopEndThreads(void) { - virNWFilterSnoopLock(); + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); + virHashRemoveSet(virNWFilterSnoopState.snoopReqs, virNWFilterSnoopRemAllReqIter, NULL); - virNWFilterSnoopUnlock(); } int @@ -1996,18 +1972,17 @@ virNWFilterDHCPSnoopInit(void) void virNWFilterDHCPSnoopEnd(const char *ifname) { + VIR_LOCK_GUARD lock = virLockGuardLock(&virNWFilterSnoopState.snoopLock); char *ifkey = NULL; - virNWFilterSnoopLock(); - if (!virNWFilterSnoopState.snoopReqs) - goto cleanup; + return; if (ifname) { ifkey = (char *)virHashLookup(virNWFilterSnoopState.ifnameToKey, ifname); if (!ifkey) - goto cleanup; + return; ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, ifname)); @@ -2020,7 +1995,7 @@ virNWFilterDHCPSnoopEnd(const char *ifname) if (!req) { virReportError(VIR_ERR_INTERNAL_ERROR, _("ifkey \"%s\" has no req"), ifkey); - goto cleanup; + return; } /* protect req->binding->portdevname & req->threadkey */ @@ -2044,9 +2019,6 @@ virNWFilterDHCPSnoopEnd(const char *ifname) virNWFilterSnoopLeaseFileLoad(); } - - cleanup: - virNWFilterSnoopUnlock(); } void @@ -2055,13 +2027,11 @@ virNWFilterDHCPSnoopShutdown(void) virNWFilterSnoopEndThreads(); virNWFilterSnoopJoinThreads(); - virNWFilterSnoopLock(); - - virNWFilterSnoopLeaseFileClose(); - g_clear_pointer(&virNWFilterSnoopState.ifnameToKey, g_hash_table_unref); - g_clear_pointer(&virNWFilterSnoopState.snoopReqs, g_hash_table_unref); - - virNWFilterSnoopUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.snoopLock) { + virNWFilterSnoopLeaseFileClose(); + g_clear_pointer(&virNWFilterSnoopState.ifnameToKey, g_hash_table_unref); + g_clear_pointer(&virNWFilterSnoopState.snoopReqs, g_hash_table_unref); + } VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.activeLock) { g_clear_pointer(&virNWFilterSnoopState.active, g_hash_table_unref); -- 2.31.1
Use automatic mutex management instead. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 273 +++++++++++------------------- 1 file changed, 95 insertions(+), 178 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index f18f7b80d6..1cbb840749 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -139,7 +139,7 @@ struct _virNWFilterSnoopReq { /* * Note about lock-order: * 1st: virNWFilterSnoopState.snoopLock - * 2nd: virNWFilterSnoopReqLock(req) + * 2nd: &req->lock * * Rationale: Former protects the SnoopReqs hash, latter its contents */ @@ -246,9 +246,6 @@ static int virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req, bool update_leasefile, bool instantiate); -static void virNWFilterSnoopReqLock(virNWFilterSnoopReq *req); -static void virNWFilterSnoopReqUnlock(virNWFilterSnoopReq *req); - static void virNWFilterSnoopLeaseFileLoad(void); static void virNWFilterSnoopLeaseFileSave(virNWFilterSnoopIPLease *ipl); @@ -362,11 +359,9 @@ virNWFilterSnoopIPLeaseTimerAdd(virNWFilterSnoopIPLease *plnew) virNWFilterSnoopReq *req = plnew->snoopReq; /* protect req->start / req->end */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); virNWFilterSnoopListAdd(plnew, &req->start, &req->end); - - virNWFilterSnoopReqUnlock(req); } /* @@ -378,12 +373,9 @@ virNWFilterSnoopIPLeaseTimerDel(virNWFilterSnoopIPLease *ipl) virNWFilterSnoopReq *req = ipl->snoopReq; /* protect req->start / req->end */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); virNWFilterSnoopListDel(ipl, &req->start, &req->end); - - virNWFilterSnoopReqUnlock(req); - ipl->timeout = 0; } @@ -401,8 +393,8 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLease *ipl, bool instantiate) { g_autofree char *ipaddr = virSocketAddrFormat(&ipl->ipAddress); - int rc = -1; virNWFilterSnoopReq *req; + VIR_LOCK_GUARD lock = { NULL }; if (!ipaddr) return -1; @@ -410,27 +402,20 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLease *ipl, req = ipl->snoopReq; /* protect req->binding->portdevname */ - virNWFilterSnoopReqLock(req); + lock = virLockGuardLock(&req->lock); if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, ipaddr) < 0) - goto cleanup; + return -1; - if (!instantiate) { - rc = 0; - goto cleanup; - } + if (!instantiate) + return 0; /* instantiate the filters */ - if (req->binding->portdevname) { - rc = virNWFilterInstantiateFilterLate(req->driver, - req->binding, - req->ifindex); - } + if (!req->binding->portdevname) + return -1; - cleanup: - virNWFilterSnoopReqUnlock(req); - return rc; + return virNWFilterInstantiateFilterLate(req->driver, req->binding, req->ifindex); } /* @@ -473,7 +458,7 @@ virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReq *req) bool is_last = false; /* protect req->start */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); while (req->start && req->start->timeout <= now) { if (req->start->next == NULL || @@ -483,8 +468,6 @@ virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReq *req) is_last); } - virNWFilterSnoopReqUnlock(req); - return 0; } @@ -562,24 +545,6 @@ virNWFilterSnoopReqFree(virNWFilterSnoopReq *req) g_free(req); } -/* - * Lock a Snoop request 'req' - */ -static void -virNWFilterSnoopReqLock(virNWFilterSnoopReq *req) -{ - virMutexLock(&req->lock); -} - -/* - * Unlock a Snoop request 'req' - */ -static void -virNWFilterSnoopReqUnlock(virNWFilterSnoopReq *req) -{ - virMutexUnlock(&req->lock); -} - /* * virNWFilterSnoopReqRelease - hash table free function to kill a request */ @@ -592,12 +557,10 @@ virNWFilterSnoopReqRelease(void *req0) return; /* protect req->threadkey */ - virNWFilterSnoopReqLock(req); - - if (req->threadkey) - virNWFilterSnoopCancel(&req->threadkey); - - virNWFilterSnoopReqUnlock(req); + VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) { + if (req->threadkey) + virNWFilterSnoopCancel(&req->threadkey); + } virNWFilterSnoopReqFree(req); } @@ -658,45 +621,33 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReq *req, virNWFilterSnoopIPLease *plnew, bool update_leasefile) { + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); virNWFilterSnoopIPLease *pl; plnew->snoopReq = req; - /* protect req->start and the lease */ - virNWFilterSnoopReqLock(req); - pl = virNWFilterSnoopIPLeaseGetByIP(req->start, &plnew->ipAddress); if (pl) { virNWFilterSnoopIPLeaseUpdate(pl, plnew->timeout); + virLockGuardUnlock(&lock); + } else { + pl = g_new0(virNWFilterSnoopIPLease, 1); + *pl = *plnew; - virNWFilterSnoopReqUnlock(req); - - goto cleanup; - } - - virNWFilterSnoopReqUnlock(req); + if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) { + g_free(pl); + return -1; + } - pl = g_new0(virNWFilterSnoopIPLease, 1); - *pl = *plnew; + virLockGuardUnlock(&lock); - /* protect req->threadkey */ - virNWFilterSnoopReqLock(req); + /* put the lease on the req's list */ + virNWFilterSnoopIPLeaseTimerAdd(pl); - if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) { - virNWFilterSnoopReqUnlock(req); - g_free(pl); - return -1; + g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1); } - virNWFilterSnoopReqUnlock(req); - - /* put the lease on the req's list */ - virNWFilterSnoopIPLeaseTimerAdd(pl); - - g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1); - - cleanup: if (update_leasefile) virNWFilterSnoopLeaseFileSave(pl); @@ -710,24 +661,19 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReq *req, static int virNWFilterSnoopReqRestore(virNWFilterSnoopReq *req) { - int ret = 0; virNWFilterSnoopIPLease *ipl; /* protect req->start */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); for (ipl = req->start; ipl; ipl = ipl->next) { /* instantiate the rules at the last lease */ bool is_last = (ipl->next == NULL); - if (virNWFilterSnoopIPLeaseInstallRule(ipl, is_last) < 0) { - ret = -1; - break; - } + if (virNWFilterSnoopIPLeaseInstallRule(ipl, is_last) < 0) + return -1; } - virNWFilterSnoopReqUnlock(req); - - return ret; + return 0; } /* @@ -759,16 +705,15 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req, g_autofree char *ipstr = NULL; /* protect req->start, req->ifname and the lease */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); ipl = virNWFilterSnoopIPLeaseGetByIP(req->start, ipaddr); if (ipl == NULL) - goto lease_not_found; + return 0; ipstr = virSocketAddrFormat(&ipl->ipAddress); if (!ipstr) { - ret = -1; - goto lease_not_found; + return -1; } virNWFilterSnoopIPLeaseTimerDel(ipl); @@ -807,10 +752,6 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req, g_free(ipl); ignore_value(!!g_atomic_int_dec_and_test(&virNWFilterSnoopState.nLeases)); - - lease_not_found: - virNWFilterSnoopReqUnlock(req); - return ret; } @@ -1279,42 +1220,36 @@ virNWFilterDHCPSnoopThread(void *req0) /* whoever started us increased the reference counter for the req for us */ /* protect req->binding->portdevname & req->threadkey */ - virNWFilterSnoopReqLock(req); - - if (req->binding->portdevname && req->threadkey) { - for (i = 0; i < G_N_ELEMENTS(pcapConf); i++) { - pcapConf[i].handle = - virNWFilterSnoopDHCPOpen(req->binding->portdevname, - &req->binding->mac, - pcapConf[i].filter, - pcapConf[i].dir); - if (!pcapConf[i].handle) { - error = true; - break; + VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) { + if (req->binding->portdevname && req->threadkey) { + for (i = 0; i < G_N_ELEMENTS(pcapConf); i++) { + pcapConf[i].handle = + virNWFilterSnoopDHCPOpen(req->binding->portdevname, + &req->binding->mac, + pcapConf[i].filter, + pcapConf[i].dir); + if (!pcapConf[i].handle) { + error = true; + break; + } + fds[i].fd = pcap_fileno(pcapConf[i].handle); } - fds[i].fd = pcap_fileno(pcapConf[i].handle); + tmp = virNetDevGetIndex(req->binding->portdevname, &ifindex); + threadkey = g_strdup(req->threadkey); + worker = virThreadPoolNewFull(1, 1, 0, virNWFilterDHCPDecodeWorker, + "dhcp-decode", NULL, req); } - tmp = virNetDevGetIndex(req->binding->portdevname, &ifindex); - threadkey = g_strdup(req->threadkey); - worker = virThreadPoolNewFull(1, 1, 0, - virNWFilterDHCPDecodeWorker, - "dhcp-decode", - NULL, - req); - } - /* let creator know how well we initialized */ - if (error || !threadkey || tmp < 0 || !worker || - ifindex != req->ifindex) { - virErrorPreserveLast(&req->threadError); - req->threadStatus = THREAD_STATUS_FAIL; - } else { - req->threadStatus = THREAD_STATUS_OK; - } - - virCondSignal(&req->threadStatusCond); + /* let creator know how well we initialized */ + if (error || !threadkey || tmp < 0 || !worker || ifindex != req->ifindex) { + virErrorPreserveLast(&req->threadError); + req->threadStatus = THREAD_STATUS_FAIL; + } else { + req->threadStatus = THREAD_STATUS_OK; + } - virNWFilterSnoopReqUnlock(req); + virCondSignal(&req->threadStatusCond); + } if (req->threadStatus != THREAD_STATUS_OK) goto cleanup; @@ -1362,12 +1297,10 @@ virNWFilterDHCPSnoopThread(void *req0) tmp = -1; /* protect req->binding->portdevname */ - virNWFilterSnoopReqLock(req); - - if (req->binding->portdevname) - tmp = virNetDevValidateConfig(req->binding->portdevname, NULL, ifindex); - - virNWFilterSnoopReqUnlock(req); + VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) { + if (req->binding->portdevname) + tmp = virNetDevValidateConfig(req->binding->portdevname, NULL, ifindex); + } if (tmp <= 0) { error = true; @@ -1378,20 +1311,17 @@ virNWFilterDHCPSnoopThread(void *req0) g_clear_pointer(&pcapConf[i].handle, pcap_close); /* protect req->binding->portdevname */ - virNWFilterSnoopReqLock(req); - - virReportError(VIR_ERR_INTERNAL_ERROR, - _("interface '%s' failing; " - "reopening"), - req->binding->portdevname); - if (req->binding->portdevname) - pcapConf[i].handle = - virNWFilterSnoopDHCPOpen(req->binding->portdevname, - &req->binding->mac, - pcapConf[i].filter, - pcapConf[i].dir); - - virNWFilterSnoopReqUnlock(req); + VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("interface '%s' failing; reopening"), + req->binding->portdevname); + if (req->binding->portdevname) + pcapConf[i].handle = + virNWFilterSnoopDHCPOpen(req->binding->portdevname, + &req->binding->mac, + pcapConf[i].filter, + pcapConf[i].dir); + } if (!pcapConf[i].handle) { error = true; @@ -1448,16 +1378,14 @@ virNWFilterDHCPSnoopThread(void *req0) /* protect IfNameToKey */ VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.snoopLock) { /* protect req->binding->portdevname & req->threadkey */ - virNWFilterSnoopReqLock(req); - - virNWFilterSnoopCancel(&req->threadkey); - - ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, - req->binding->portdevname)); + VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) { + virNWFilterSnoopCancel(&req->threadkey); - g_clear_pointer(&req->binding->portdevname, g_free); + ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, + req->binding->portdevname)); - virNWFilterSnoopReqUnlock(req); + g_clear_pointer(&req->binding->portdevname, g_free); + } } cleanup: @@ -1497,6 +1425,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, virNWFilterVarValue *dhcpsrvrs; bool threadPuts = false; VIR_LOCK_GUARD lock = { NULL }; + VIR_LOCK_GUARD lockReq = { NULL }; virNWFilterSnoopIFKeyFMT(ifkey, binding->owneruuid, &binding->mac); @@ -1564,7 +1493,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, } /* prevent thread from holding req */ - virNWFilterSnoopReqLock(req); + lockReq = virLockGuardLock(&req->lock); if (virThreadCreateFull(&thread, false, virNWFilterDHCPSnoopThread, "dhcp-snoop", false, req) != 0) { @@ -1605,8 +1534,6 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, goto exit_snoop_cancel; } - virNWFilterSnoopReqUnlock(req); - /* do not 'put' the req -- the thread will do this */ return 0; @@ -1614,7 +1541,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver, exit_snoop_cancel: virNWFilterSnoopCancel(&req->threadkey); exit_snoopreq_unlock: - virNWFilterSnoopReqUnlock(req); + virLockGuardUnlock(&lockReq); exit_rem_ifnametokey: virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, binding->portdevname); exit_snoopunlock: @@ -1705,12 +1632,11 @@ virNWFilterSnoopPruneIter(const void *payload, const void *data G_GNUC_UNUSED) { virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload; - bool del_req; /* clean up orphaned, expired leases */ /* protect req->threadkey */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); if (!req->threadkey) virNWFilterSnoopReqLeaseTimerRun(req); @@ -1718,11 +1644,7 @@ virNWFilterSnoopPruneIter(const void *payload, /* * have the entry removed if it has no leases and no one holds a ref */ - del_req = ((req->start == NULL) && (g_atomic_int_get(&req->refctr) == 0)); - - virNWFilterSnoopReqUnlock(req); - - return del_req; + return ((req->start == NULL) && (g_atomic_int_get(&req->refctr) == 0)); } /* @@ -1739,12 +1661,11 @@ virNWFilterSnoopSaveIter(void *payload, virNWFilterSnoopIPLease *ipl; /* protect req->start */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); for (ipl = req->start; ipl; ipl = ipl->next) ignore_value(virNWFilterSnoopLeaseFileWrite(tfd, req->ifkey, ipl)); - virNWFilterSnoopReqUnlock(req); return 0; } @@ -1900,7 +1821,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload, virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload; /* protect req->binding->portdevname */ - virNWFilterSnoopReqLock(req); + VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock); if (req->binding && req->binding->portdevname) { ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, @@ -1916,8 +1837,6 @@ virNWFilterSnoopRemAllReqIter(const void *payload, g_clear_pointer(&req->binding->portdevname, g_free); } - virNWFilterSnoopReqUnlock(req); - /* removal will call virNWFilterSnoopCancel() */ return 1; } @@ -1999,14 +1918,12 @@ virNWFilterDHCPSnoopEnd(const char *ifname) } /* protect req->binding->portdevname & req->threadkey */ - virNWFilterSnoopReqLock(req); - - /* keep valid lease req; drop interface association */ - virNWFilterSnoopCancel(&req->threadkey); + VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) { + /* keep valid lease req; drop interface association */ + virNWFilterSnoopCancel(&req->threadkey); - g_clear_pointer(&req->binding->portdevname, g_free); - - virNWFilterSnoopReqUnlock(req); + g_clear_pointer(&req->binding->portdevname, g_free); + } virNWFilterSnoopReqPut(req); } else { /* free all of them */ -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_learnipaddr.c | 83 +++++++++-------------------- 1 file changed, 24 insertions(+), 59 deletions(-) diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 99bffdc4fb..2c85972012 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -143,11 +143,9 @@ static bool threadsTerminate; int virNWFilterLockIface(const char *ifname) { - virNWFilterIfaceLock *ifaceLock; + VIR_LOCK_GUARD lock = virLockGuardLock(&ifaceMapLock); + virNWFilterIfaceLock *ifaceLock = virHashLookup(ifaceLockMap, ifname); - virMutexLock(&ifaceMapLock); - - ifaceLock = virHashLookup(ifaceLockMap, ifname); if (!ifaceLock) { ifaceLock = g_new0(virNWFilterIfaceLock, 1); @@ -155,21 +153,20 @@ virNWFilterLockIface(const char *ifname) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mutex initialization failed")); g_free(ifaceLock); - goto error; + return -1; } if (virStrcpyStatic(ifaceLock->ifname, ifname) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("interface name %s does not fit into " - "buffer "), + _("interface name %s does not fit into buffer"), ifaceLock->ifname); g_free(ifaceLock); - goto error; + return -1; } while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) { g_free(ifaceLock); - goto error; + return -1; } ifaceLock->refctr = 0; @@ -177,27 +174,17 @@ virNWFilterLockIface(const char *ifname) ifaceLock->refctr++; - virMutexUnlock(&ifaceMapLock); - virMutexLock(&ifaceLock->lock); return 0; - - error: - virMutexUnlock(&ifaceMapLock); - - return -1; } void virNWFilterUnlockIface(const char *ifname) { - virNWFilterIfaceLock *ifaceLock; - - virMutexLock(&ifaceMapLock); - - ifaceLock = virHashLookup(ifaceLockMap, ifname); + VIR_LOCK_GUARD lock = virLockGuardLock(&ifaceMapLock); + virNWFilterIfaceLock *ifaceLock = virHashLookup(ifaceLockMap, ifname); if (ifaceLock) { virMutexUnlock(&ifaceLock->lock); @@ -206,8 +193,6 @@ virNWFilterUnlockIface(const char *ifname) if (ifaceLock->refctr == 0) virHashRemoveEntry(ifaceLockMap, ifname); } - - virMutexUnlock(&ifaceMapLock); } @@ -228,17 +213,13 @@ virNWFilterIPAddrLearnReqFree(virNWFilterIPAddrLearnReq *req) static int virNWFilterRegisterLearnReq(virNWFilterIPAddrLearnReq *req) { - int res = -1; g_autofree char *ifindex_str = g_strdup_printf("%d", req->ifindex); + VIR_LOCK_GUARD lock = virLockGuardLock(&pendingLearnReqLock); - virMutexLock(&pendingLearnReqLock); - - if (!virHashLookup(pendingLearnReq, ifindex_str)) - res = virHashAddEntry(pendingLearnReq, ifindex_str, req); - - virMutexUnlock(&pendingLearnReqLock); + if (virHashLookup(pendingLearnReq, ifindex_str)) + return -1; - return res; + return virHashAddEntry(pendingLearnReq, ifindex_str, req); } @@ -247,9 +228,7 @@ virNWFilterRegisterLearnReq(virNWFilterIPAddrLearnReq *req) int virNWFilterTerminateLearnReq(const char *ifname) { - int rc = -1; int ifindex; - virNWFilterIPAddrLearnReq *req; g_autofree char *ifindex_str = NULL; /* It's possible that it's already been removed as a result of @@ -262,38 +241,30 @@ virNWFilterTerminateLearnReq(const char *ifname) if (virNetDevGetIndex(ifname, &ifindex) < 0) { virResetLastError(); - return rc; + return -1; } ifindex_str = g_strdup_printf("%d", ifindex); - virMutexLock(&pendingLearnReqLock); - - req = virHashLookup(pendingLearnReq, ifindex_str); - if (req) { - rc = 0; - req->terminate = true; + VIR_WITH_MUTEX_LOCK_GUARD(&pendingLearnReqLock) { + virNWFilterIPAddrLearnReq *req; + if ((req = virHashLookup(pendingLearnReq, ifindex_str))) { + req->terminate = true; + return 0; + } } - virMutexUnlock(&pendingLearnReqLock); - - return rc; + return -1; } bool virNWFilterHasLearnReq(int ifindex) { - void *res; g_autofree char *ifindex_str = g_strdup_printf("%d", ifindex); + VIR_LOCK_GUARD lock = virLockGuardLock(&pendingLearnReqLock); - virMutexLock(&pendingLearnReqLock); - - res = virHashLookup(pendingLearnReq, ifindex_str); - - virMutexUnlock(&pendingLearnReqLock); - - return res != NULL; + return virHashLookup(pendingLearnReq, ifindex_str) != NULL; } @@ -309,16 +280,10 @@ freeLearnReqEntry(void *payload) static virNWFilterIPAddrLearnReq * virNWFilterDeregisterLearnReq(int ifindex) { - virNWFilterIPAddrLearnReq *res; g_autofree char *ifindex_str = g_strdup_printf("%d", ifindex); + VIR_LOCK_GUARD lock = virLockGuardLock(&pendingLearnReqLock); - virMutexLock(&pendingLearnReqLock); - - res = virHashSteal(pendingLearnReq, ifindex_str); - - virMutexUnlock(&pendingLearnReqLock); - - return res; + return virHashSteal(pendingLearnReq, ifindex_str); } #endif -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_ipaddrmap.c | 80 ++++++++++++++--------------------- 1 file changed, 31 insertions(+), 49 deletions(-) diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c index e2f123b9d9..4090cc2769 100644 --- a/src/conf/nwfilter_ipaddrmap.c +++ b/src/conf/nwfilter_ipaddrmap.c @@ -49,37 +49,28 @@ static GHashTable *ipAddressMap; int virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr) { - int ret = -1; - char *addrCopy; + g_autofree char *addrCopy = g_strdup(addr); + VIR_LOCK_GUARD lock = virLockGuardLock(&ipAddressMapLock); virNWFilterVarValue *val; - addrCopy = g_strdup(addr); - - virMutexLock(&ipAddressMapLock); - - val = virHashLookup(ipAddressMap, ifname); - if (!val) { - val = virNWFilterVarValueCreateSimple(addrCopy); - if (!val) - goto cleanup; - addrCopy = NULL; - ret = virHashUpdateEntry(ipAddressMap, ifname, val); - if (ret < 0) - virNWFilterVarValueFree(val); - goto cleanup; - } else { + if ((val = virHashLookup(ipAddressMap, ifname)) != NULL) { if (virNWFilterVarValueAddValue(val, addrCopy) < 0) - goto cleanup; + return -1; + addrCopy = NULL; + return 0; } - ret = 0; + if ((val = virNWFilterVarValueCreateSimple(addrCopy)) == NULL) + return -1; - cleanup: - virMutexUnlock(&ipAddressMapLock); - VIR_FREE(addrCopy); + addrCopy = NULL; + if (virHashUpdateEntry(ipAddressMap, ifname, val) < 0) { + virNWFilterVarValueFree(val); + return -1; + } - return ret; + return 0; } /* Delete all or a specific IP address from an interface. After this @@ -99,31 +90,28 @@ virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr) int virNWFilterIPAddrMapDelIPAddr(const char *ifname, const char *ipaddr) { - int ret = -1; + VIR_LOCK_GUARD lock = virLockGuardLock(&ipAddressMapLock); virNWFilterVarValue *val = NULL; - virMutexLock(&ipAddressMapLock); - - if (ipaddr != NULL) { - val = virHashLookup(ipAddressMap, ifname); - if (val) { - if (virNWFilterVarValueGetCardinality(val) == 1 && - STREQ(ipaddr, - virNWFilterVarValueGetNthValue(val, 0))) - goto remove_entry; - virNWFilterVarValueDelValue(val, ipaddr); - ret = virNWFilterVarValueGetCardinality(val); - } - } else { - remove_entry: + if (!ipaddr) { /* remove whole entry */ virHashRemoveEntry(ipAddressMap, ifname); - ret = 0; + return 0; + } + + if (!(val = virHashLookup(ipAddressMap, ifname))) { + return -1; } - virMutexUnlock(&ipAddressMapLock); + if (virNWFilterVarValueGetCardinality(val) == 1 && + STREQ(ipaddr, virNWFilterVarValueGetNthValue(val, 0))) { + /* remove whole entry */ + virHashRemoveEntry(ipAddressMap, ifname); + return 0; + } - return ret; + virNWFilterVarValueDelValue(val, ipaddr); + return virNWFilterVarValueGetCardinality(val); } /* Get the list of IP addresses known to be in use by an interface @@ -135,15 +123,9 @@ virNWFilterIPAddrMapDelIPAddr(const char *ifname, const char *ipaddr) virNWFilterVarValue * virNWFilterIPAddrMapGetIPAddr(const char *ifname) { - virNWFilterVarValue *res; - - virMutexLock(&ipAddressMapLock); - - res = virHashLookup(ipAddressMap, ifname); - - virMutexUnlock(&ipAddressMapLock); + VIR_LOCK_GUARD lock = virLockGuardLock(&ipAddressMapLock); - return res; + return virHashLookup(ipAddressMap, ifname); } int -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virnetlink.c | 101 ++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 62 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 3216765492..f15bb68b02 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -799,18 +799,6 @@ virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen) } -static void -virNetlinkEventServerLock(virNetlinkEventSrvPrivate *driver) -{ - virMutexLock(&driver->lock); -} - -static void -virNetlinkEventServerUnlock(virNetlinkEventSrvPrivate *driver) -{ - virMutexUnlock(&driver->lock); -} - /** * virNetlinkEventRemoveClientPrimitive: * @@ -857,6 +845,7 @@ virNetlinkEventCallback(int watch, int length; bool handled = false; g_autofree struct nlmsghdr *msg = NULL; + VIR_LOCK_GUARD lock = { NULL }; length = nl_recv(srv->netlinknh, &peer, (unsigned char **)&msg, &creds); @@ -869,7 +858,7 @@ virNetlinkEventCallback(int watch, return; } - virNetlinkEventServerLock(srv); + lock = virLockGuardLock(&srv->lock); VIR_DEBUG("dispatching to max %d clients, called from event watch %d", (int)srv->handlesCount, watch); @@ -886,8 +875,6 @@ virNetlinkEventCallback(int watch, if (!handled) VIR_DEBUG("event not handled."); - - virNetlinkEventServerUnlock(srv); } /** @@ -916,20 +903,20 @@ virNetlinkEventServiceStop(unsigned int protocol) if (!server[protocol]) return 0; - virNetlinkEventServerLock(srv); - nl_close(srv->netlinknh); - virNetlinkFree(srv->netlinknh); - virEventRemoveHandle(srv->eventwatch); + VIR_WITH_MUTEX_LOCK_GUARD(&srv->lock) { + nl_close(srv->netlinknh); + virNetlinkFree(srv->netlinknh); + virEventRemoveHandle(srv->eventwatch); - /* free any remaining clients on the list */ - for (i = 0; i < srv->handlesCount; i++) { - if (srv->handles[i].deleted == VIR_NETLINK_HANDLE_VALID) - virNetlinkEventRemoveClientPrimitive(i, protocol); - } + /* free any remaining clients on the list */ + for (i = 0; i < srv->handlesCount; i++) { + if (srv->handles[i].deleted == VIR_NETLINK_HANDLE_VALID) + virNetlinkEventRemoveClientPrimitive(i, protocol); + } - server[protocol] = NULL; - VIR_FREE(srv->handles); - virNetlinkEventServerUnlock(srv); + server[protocol] = NULL; + VIR_FREE(srv->handles); + } virMutexDestroy(&srv->lock); VIR_FREE(srv); @@ -1014,9 +1001,9 @@ int virNetlinkEventServiceLocalPid(unsigned int protocol) int virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) { - virNetlinkEventSrvPrivate *srv; + g_autofree virNetlinkEventSrvPrivate *srv = NULL; + VIR_LOCK_GUARD lock = { NULL }; int fd; - int ret = -1; if (protocol >= MAX_LINKS) { virReportSystemError(EINVAL, @@ -1031,34 +1018,32 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) srv = g_new0(virNetlinkEventSrvPrivate, 1); - if (virMutexInit(&srv->lock) < 0) { - VIR_FREE(srv); + if (virMutexInit(&srv->lock) < 0) return -1; - } - virNetlinkEventServerLock(srv); + lock = virLockGuardLock(&srv->lock); /* Allocate a new socket and get fd */ if (!(srv->netlinknh = virNetlinkCreateSocket(protocol))) - goto error_locked; + goto error; fd = nl_socket_get_fd(srv->netlinknh); if (fd < 0) { virReportSystemError(errno, "%s", _("cannot get netlink socket fd")); - goto error_server; + goto error; } if (groups && nl_socket_add_membership(srv->netlinknh, groups) < 0) { virReportSystemError(errno, "%s", _("cannot add netlink membership")); - goto error_server; + goto error; } if (nl_socket_set_nonblocking(srv->netlinknh)) { virReportSystemError(errno, "%s", _("cannot set netlink socket nonblocking")); - goto error_server; + goto error; } if ((srv->eventwatch = virEventAddHandle(fd, @@ -1067,27 +1052,24 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) srv, NULL)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to add netlink event handle watch")); - goto error_server; + goto error; } srv->netlinkfd = fd; VIR_DEBUG("netlink event listener on fd: %i running", fd); - ret = 0; - server[protocol] = srv; + server[protocol] = g_steal_pointer(&srv); + return 0; - error_server: - if (ret < 0) { + error: + if (srv->netlinknh) { nl_close(srv->netlinknh); virNetlinkFree(srv->netlinknh); } - error_locked: - virNetlinkEventServerUnlock(srv); - if (ret < 0) { - virMutexDestroy(&srv->lock); - VIR_FREE(srv); - } - return ret; + + virLockGuardUnlock(&lock); + virMutexDestroy(&srv->lock); + return -1; } /** @@ -1114,8 +1096,9 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB, unsigned int protocol) { size_t i; - int r, ret = -1; + int r; virNetlinkEventSrvPrivate *srv = NULL; + VIR_LOCK_GUARD lock = { NULL }; if (protocol >= MAX_LINKS) return -EINVAL; @@ -1128,7 +1111,7 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB, return -1; } - virNetlinkEventServerLock(srv); + lock = virLockGuardLock(&srv->lock); VIR_DEBUG("adding client: %d.", nextWatch); @@ -1163,10 +1146,7 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB, VIR_DEBUG("added client to loop slot: %d. with macaddr ptr=%p", r, macaddr); - ret = nextWatch++; - - virNetlinkEventServerUnlock(srv); - return ret; + return nextWatch++; } /** @@ -1187,8 +1167,8 @@ virNetlinkEventRemoveClient(int watch, const virMacAddr *macaddr, unsigned int protocol) { size_t i; - int ret = -1; virNetlinkEventSrvPrivate *srv = NULL; + VIR_LOCK_GUARD lock = { NULL }; if (protocol >= MAX_LINKS) return -EINVAL; @@ -1202,7 +1182,7 @@ virNetlinkEventRemoveClient(int watch, const virMacAddr *macaddr, return -1; } - virNetlinkEventServerLock(srv); + lock = virLockGuardLock(&srv->lock); for (i = 0; i < srv->handlesCount; i++) { if (srv->handles[i].deleted != VIR_NETLINK_HANDLE_VALID) @@ -1215,15 +1195,12 @@ virNetlinkEventRemoveClient(int watch, const virMacAddr *macaddr, VIR_DEBUG("removed client: %d by %s.", srv->handles[i].watch, watch ? "index" : "mac"); virNetlinkEventRemoveClientPrimitive(i, protocol); - ret = 0; - goto cleanup; + return 0; } } VIR_DEBUG("no client found to remove."); - cleanup: - virNetlinkEventServerUnlock(srv); - return ret; + return -1; } #else -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/remote/remote_daemon_stream.c | 34 +++++++++++++------------------ 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index eb7ed5edf3..a6d2c915e6 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -119,8 +119,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) virNetServerClient *client = opaque; daemonClientStream *stream; daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); - - virMutexLock(&priv->lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock); stream = priv->streams; while (stream) { @@ -132,7 +131,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) if (!stream) { VIR_WARN("event for client=%p stream st=%p, but missing stream state", client, st); virStreamEventRemoveCallback(st); - goto cleanup; + return; } VIR_DEBUG("st=%p events=%d EOF=%d closed=%d", st, events, stream->recvEOF, stream->closed); @@ -142,7 +141,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) if (daemonStreamHandleWrite(client, stream) < 0) { daemonRemoveClientStream(client, stream); virNetServerClientClose(client); - goto cleanup; + return; } } @@ -152,7 +151,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) if (daemonStreamHandleRead(client, stream) < 0) { daemonRemoveClientStream(client, stream); virNetServerClientClose(client); - goto cleanup; + return; } /* If we detected EOF during read processing, * then clear hangup/error conditions, since @@ -177,7 +176,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) virNetMessageFree(msg); daemonRemoveClientStream(client, stream); virNetServerClientClose(client); - goto cleanup; + return; } break; case VIR_NET_ERROR: @@ -187,7 +186,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) virNetMessageFree(msg); daemonRemoveClientStream(client, stream); virNetServerClientClose(client); - goto cleanup; + return; } break; } @@ -206,7 +205,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) if (!(msg = virNetMessageNew(false))) { daemonRemoveClientStream(client, stream); virNetServerClientClose(client); - goto cleanup; + return; } msg->cb = daemonStreamMessageFinished; msg->opaque = stream; @@ -220,7 +219,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) virNetMessageFree(msg); daemonRemoveClientStream(client, stream); virNetServerClientClose(client); - goto cleanup; + return; } } @@ -263,7 +262,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) daemonRemoveClientStream(client, stream); if (ret < 0) virNetServerClientClose(client); - goto cleanup; + return; } if (stream->closed) { @@ -271,9 +270,6 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) } else { daemonStreamUpdateEvents(stream); } - - cleanup: - virMutexUnlock(&priv->lock); } @@ -460,13 +456,11 @@ int daemonAddClientStream(virNetServerClient *client, if (transmit) stream->tx = true; - virMutexLock(&priv->lock); - stream->next = priv->streams; - priv->streams = stream; - - daemonStreamUpdateEvents(stream); - - virMutexUnlock(&priv->lock); + VIR_WITH_MUTEX_LOCK_GUARD(&priv->lock) { + stream->next = priv->streams; + priv->streams = stream; + daemonStreamUpdateEvents(stream); + } return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_conf.c | 70 ++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 81449b8b77..4b5f75b694 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -85,17 +85,6 @@ static int virQEMUConfigOnceInit(void) VIR_ONCE_GLOBAL_INIT(virQEMUConfig); -static void -qemuDriverLock(virQEMUDriver *driver) -{ - virMutexLock(&driver->lock); -} -static void -qemuDriverUnlock(virQEMUDriver *driver) -{ - virMutexUnlock(&driver->lock); -} - #ifndef DEFAULT_LOADER_NVRAM # define DEFAULT_LOADER_NVRAM \ "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd:" \ @@ -1279,11 +1268,9 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfig *cfg) virQEMUDriverConfig *virQEMUDriverGetConfig(virQEMUDriver *driver) { - virQEMUDriverConfig *conf; - qemuDriverLock(driver); - conf = virObjectRef(driver->config); - qemuDriverUnlock(driver); - return conf; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + + return virObjectRef(driver->config); } virDomainXMLOption * @@ -1303,16 +1290,13 @@ virQEMUDriverCreateXMLConf(virQEMUDriver *driver, virCPUDef * virQEMUDriverGetHostCPU(virQEMUDriver *driver) { - virCPUDef *hostcpu; - - qemuDriverLock(driver); - - if (!driver->hostcpu) - driver->hostcpu = virCPUProbeHost(virArchFromHost()); + virCPUDef *hostcpu = NULL; - hostcpu = driver->hostcpu; - - qemuDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + if (!driver->hostcpu) + driver->hostcpu = virCPUProbeHost(virArchFromHost()); + hostcpu = driver->hostcpu; + } if (hostcpu) virCPUDefRef(hostcpu); @@ -1389,32 +1373,36 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver) * Returns: a reference to a virCaps *instance or NULL */ virCaps *virQEMUDriverGetCapabilities(virQEMUDriver *driver, - bool refresh) + bool refresh) { + bool force_refresh = false; virCaps *ret = NULL; + if (refresh) { virCaps *caps = NULL; if ((caps = virQEMUDriverCreateCapabilities(driver)) == NULL) return NULL; - qemuDriverLock(driver); - virObjectUnref(driver->caps); - driver->caps = caps; - } else { - qemuDriverLock(driver); - - if (driver->caps == NULL || - driver->caps->nguests == 0) { - VIR_DEBUG("Capabilities didn't detect any guests. Forcing a " - "refresh."); - qemuDriverUnlock(driver); - return virQEMUDriverGetCapabilities(driver, true); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + virObjectUnref(driver->caps); + driver->caps = caps; + ret = virObjectRef(driver->caps); } + + return ret; + } + + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + force_refresh = driver->caps == NULL || driver->caps->nguests == 0; + if (!force_refresh) + ret = virObjectRef(driver->caps); } - ret = virObjectRef(driver->caps); - qemuDriverUnlock(driver); - return ret; + if (!force_refresh) + return ret; + + VIR_DEBUG("Capabilities didn't detect any guests. Forcing a refresh."); + return virQEMUDriverGetCapabilities(driver, true); } -- 2.31.1
participants (2)
-
Daniel P. Berrangé -
Tim Wiederhake