[libvirt] [PATCH 0/7] Misc improvements
As I started to turn more object into using RW locks, I've found couple of areas for improvement too. Michal Privoznik (7): virConnect: Update comment for @privateData Report error if virMutexInit fails virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static again virNetworkObjList: Derive from virObjectRWLockable virNodeDeviceObjList: Derive from virObjectRWLockable virConnect: Derive from virObjectRWLockable storageDriver: Use RW locks src/bhyve/bhyve_driver.c | 1 + src/conf/virnetworkobj.c | 42 ++++++++++---------------------- src/conf/virnetworkobj.h | 8 ------- src/conf/virnodedeviceobj.c | 16 ++++++------- src/conf/virstorageobj.h | 2 +- src/datatypes.c | 6 +++-- src/datatypes.h | 6 ++--- src/libvirt_private.syms | 2 -- src/lxc/lxc_driver.c | 1 + src/lxc/lxc_fuse.c | 4 +++- src/network/bridge_driver.c | 1 + src/node_device/node_device_hal.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 12 +++++++--- src/nwfilter/nwfilter_driver.c | 5 +++- src/nwfilter/nwfilter_gentech_driver.c | 4 +++- src/secret/secret_driver.c | 2 ++ src/storage/storage_driver.c | 44 +++++++++++++++++++--------------- src/uml/uml_driver.c | 1 + src/util/virerror.c | 2 +- src/util/virnetlink.c | 1 + src/util/virthreadpool.c | 4 +++- src/vmware/vmware_driver.c | 5 +++- src/vz/vz_driver.c | 4 +++- tools/virsh-console.c | 4 +++- 24 files changed, 94 insertions(+), 84 deletions(-) -- 2.13.0
This member allows us to store a pointer to some private data. However, the comment says it's used in both domain driver and network driver. Well, it is not. It's just one pointer and domain driver uses it directly. Network driver has a global driver variable. Update the comment to not confuse others. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/datatypes.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/datatypes.h b/src/datatypes.h index 288e057cc..8a0399cd0 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -472,8 +472,8 @@ struct _virConnect { virSecretDriverPtr secretDriver; virNWFilterDriverPtr nwfilterDriver; - /* Private data pointer which can be used by driver and - * network driver as they wish. + /* Private data pointer which can be used by domain driver as + * it pleases. * NB: 'private' is a reserved word in C++. */ void * privateData; -- 2.13.0
On Thu, Jul 27, 2017 at 01:47:21PM +0200, Michal Privoznik wrote:
This member allows us to store a pointer to some private data. However, the comment says it's used in both domain driver and network driver. Well, it is not. It's just one pointer and domain driver uses it directly. Network driver has a global driver variable. Update the comment to not confuse others.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/datatypes.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK, trivial
diff --git a/src/datatypes.h b/src/datatypes.h index 288e057cc..8a0399cd0 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -472,8 +472,8 @@ struct _virConnect { virSecretDriverPtr secretDriver; virNWFilterDriverPtr nwfilterDriver;
- /* Private data pointer which can be used by driver and - * network driver as they wish. + /* Private data pointer which can be used by domain driver as + * it pleases. * NB: 'private' is a reserved word in C++. */ void * privateData; -- 2.13.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
The virMutexInit() function is not reporting any error on failure rather than returning -1 and setting errno. It's up to the caller to report the error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/lxc/lxc_fuse.c | 4 +++- src/network/bridge_driver.c | 1 + src/node_device/node_device_hal.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 12 +++++++++--- src/nwfilter/nwfilter_driver.c | 5 ++++- src/nwfilter/nwfilter_gentech_driver.c | 4 +++- src/secret/secret_driver.c | 2 ++ src/storage/storage_driver.c | 2 ++ src/uml/uml_driver.c | 1 + src/util/virnetlink.c | 1 + src/util/virthreadpool.c | 4 +++- src/vmware/vmware_driver.c | 5 ++++- src/vz/vz_driver.c | 4 +++- tools/virsh-console.c | 4 +++- 16 files changed, 42 insertions(+), 10 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 550b257cd..cb22842f5 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1240,6 +1240,7 @@ bhyveStateInitialize(bool privileged, return -1; if (virMutexInit(&bhyve_driver->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init bhyve driver lock")); VIR_FREE(bhyve_driver); return -1; } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 6eb88b0ba..6cd818a08 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1631,6 +1631,7 @@ static int lxcStateInitialize(bool privileged, if (VIR_ALLOC(lxc_driver) < 0) return -1; if (virMutexInit(&lxc_driver->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init lxc driver lock")); VIR_FREE(lxc_driver); return -1; } diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c index 60d41243a..dca381152 100644 --- a/src/lxc/lxc_fuse.c +++ b/src/lxc/lxc_fuse.c @@ -309,8 +309,10 @@ int lxcSetupFuse(virLXCFusePtr *f, virDomainDefPtr def) fuse->def = def; - if (virMutexInit(&fuse->lock) < 0) + if (virMutexInit(&fuse->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init fuse lock")); goto cleanup2; + } if (virAsprintf(&fuse->mountpoint, "%s/%s.fuse/", LXC_STATE_DIR, def->name) < 0) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d05e08fc9..c26ee6dcc 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -729,6 +729,7 @@ networkStateInitialize(bool privileged, goto error; if (virMutexInit(&network_driver->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init network driver lock")); VIR_FREE(network_driver); goto error; } diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index c19e327c9..fcbbb4e6d 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -612,6 +612,7 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, return -1; if (virMutexInit(&driver->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init node driver lock")); VIR_FREE(driver); return -1; } diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 4436e396f..b7bf913c9 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -592,10 +592,14 @@ virNWFilterSnoopReqNew(const char *ifkey) req->threadStatus = THREAD_STATUS_NONE; - if (virStrcpyStatic(req->ifkey, ifkey) == NULL || - virMutexInitRecursive(&req->lock) < 0) + if (virStrcpyStatic(req->ifkey, ifkey) == NULL) goto err_free_req; + if (virMutexInitRecursive(&req->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init nwfilter lock")); + goto err_free_req; + } + if (virCondInit(&req->threadStatusCond) < 0) goto err_destroy_mutex; @@ -2085,8 +2089,10 @@ virNWFilterDHCPSnoopInit(void) VIR_DEBUG("Initializing DHCP snooping"); if (virMutexInitRecursive(&virNWFilterSnoopState.snoopLock) < 0 || - virMutexInit(&virNWFilterSnoopState.activeLock) < 0) + virMutexInit(&virNWFilterSnoopState.activeLock) < 0) { + virReportSystemError(errno, "%s", _("unable to init nwfilter lock")); return -1; + } virNWFilterSnoopState.ifnameToKey = virHashCreate(0, NULL); virNWFilterSnoopState.active = virHashCreate(0, NULL); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2f9a51c40..12e11a1c8 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -183,8 +183,11 @@ nwfilterStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) return -1; - if (virMutexInit(&driver->lock) < 0) + if (virMutexInit(&driver->lock) < 0) { + virReportSystemError(errno, "%s", + _("unable to init nwfilter driver lock")); goto err_free_driverstate; + } /* remember that we are going to use firewalld */ driver->watchingFirewallD = (sysbus != NULL); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 6758200b5..c0b07cff2 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -77,8 +77,10 @@ int virNWFilterTechDriversInit(bool privileged) { size_t i = 0; VIR_DEBUG("Initializing NWFilter technology drivers"); - if (virMutexInitRecursive(&updateMutex) < 0) + if (virMutexInitRecursive(&updateMutex) < 0) { + virReportSystemError(errno, "%s", _("unable to init nwfilter lock")); return -1; + } while (filter_tech_drivers[i]) { if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index d833a863f..3848cfb81 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -458,6 +458,8 @@ secretStateInitialize(bool privileged, return -1; if (virMutexInit(&driver->lock) < 0) { + virReportSystemError(errno, "%s", + _("unable to init secret driver lock")); VIR_FREE(driver); return -1; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 855212063..f162f2e5a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -254,6 +254,8 @@ storageStateInitialize(bool privileged, return ret; if (virMutexInit(&driver->lock) < 0) { + virReportSystemError(errno, "%s", + _("cannot initialize storage driver lock")); VIR_FREE(driver); return ret; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 1846835cc..62d78a3ee 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -473,6 +473,7 @@ umlStateInitialize(bool privileged, uml_driver->inhibitOpaque = opaque; if (virMutexInit(¨_driver->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init uml driver lock")); VIR_FREE(uml_driver); return -1; } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d732fe8cf..f297207a3 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -862,6 +862,7 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) return -1; if (virMutexInit(&srv->lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init netlink lock")); VIR_FREE(srv); return -1; } diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 10f2bd2c3..d9e17346f 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -239,8 +239,10 @@ virThreadPoolNewFull(size_t minWorkers, pool->jobFuncName = funcName; pool->jobOpaque = opaque; - if (virMutexInit(&pool->mutex) < 0) + if (virMutexInit(&pool->mutex) < 0) { + virReportSystemError(errno, "%s", _("unable to init thread pool lock")); goto error; + } if (virCondInit(&pool->cond) < 0) goto error; if (virCondInit(&pool->quit_cond) < 0) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 8b487c4a7..eed6c865b 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -176,8 +176,11 @@ vmwareConnectOpen(virConnectPtr conn, goto cleanup; } - if (virMutexInit(&driver->lock) < 0) + if (virMutexInit(&driver->lock) < 0) { + virReportSystemError(errno, "%s", + _("unable to init vmware driver lock")); goto cleanup; + } if ((tmp = STRSKIP(conn->uri->scheme, "vmware")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to parse URI " diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 6f4aee365..fab01e1c3 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -4133,8 +4133,10 @@ vzStateInitialize(bool privileged ATTRIBUTE_UNUSED, return -1; } - if (virMutexInit(&vz_driver_lock) < 0) + if (virMutexInit(&vz_driver_lock) < 0) { + virReportSystemError(errno, "%s", _("unable to init vz driver lock")); goto error; + } /* Failing to create driver here is not fatal and only means * that next driver client will try once more when connecting */ diff --git a/tools/virsh-console.c b/tools/virsh-console.c index c1927c28a..ab3339ec7 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -351,8 +351,10 @@ virshRunConsole(vshControl *ctl, if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0) goto cleanup; - if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0) + if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0) { + VIR_ERROR(_("unable to init console lock or condition")); goto cleanup; + } virMutexLock(&con->lock); -- 2.13.0
On Thu, Jul 27, 2017 at 01:47:22PM +0200, Michal Privoznik wrote:
The virMutexInit() function is not reporting any error on failure rather than returning -1 and setting errno. It's up to the caller to report the error.
I would rather see virMutexInit() report the error since it looks like some code already counts on it (I haven't done any comparison) and different error messages for each mutex does not really help anything IMHO since mutex initialization is fatal anyway. I think virthread.c does not report errors because we had multiple implementations for it, but that's no longer true since 404174cad321 (late 2012).
diff --git a/tools/virsh-console.c b/tools/virsh-console.c index c1927c28a..ab3339ec7 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -351,8 +351,10 @@ virshRunConsole(vshControl *ctl, if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0) goto cleanup;
- if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0) + if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0) { + VIR_ERROR(_("unable to init console lock or condition"));
I don't see a reason for VIR_ERROR here. Also virCondInit() and other could report error, although that's not the aim of this patch.
goto cleanup; + }
virMutexLock(&con->lock);
-- 2.13.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
These functions were made exportable back in 3aa3e072 when I was splitting network code into parsing and list management parts. Since then the split is finished now and these two functions do not need to be exported anymore. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virnetworkobj.c | 22 ++-------------------- src/conf/virnetworkobj.h | 8 -------- src/libvirt_private.syms | 2 -- 3 files changed, 2 insertions(+), 30 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index ccde72e72..d8eca1e90 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -127,16 +127,7 @@ virNetworkObjListNew(void) } -/** - * virNetworkObjFindByUUIDLocked: - * @nets: list of network objects - * @uuid: network uuid to find - * - * This functions requires @nets to be locked already! - * - * Returns: not locked, but ref'd network object. - */ -virNetworkObjPtr +static virNetworkObjPtr virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, const unsigned char *uuid) { @@ -193,16 +184,7 @@ virNetworkObjSearchName(const void *payload, } -/* - * virNetworkObjFindByNameLocked: - * @nets: list of network objects - * @name: network name to find - * - * This functions requires @nets to be locked already! - * - * Returns: not locked, but ref'd network object. - */ -virNetworkObjPtr +static virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, const char *name) { diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h index 8090c2e24..cb1fd13a3 100644 --- a/src/conf/virnetworkobj.h +++ b/src/conf/virnetworkobj.h @@ -65,18 +65,10 @@ virNetworkObjIsActive(const virNetworkObj *net) virNetworkObjListPtr virNetworkObjListNew(void); -virNetworkObjPtr -virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, - const unsigned char *uuid); - virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid); -virNetworkObjPtr -virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, - const char *name); - virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 37b815c06..054315fb7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -941,9 +941,7 @@ virNetworkObjBridgeInUse; virNetworkObjDeleteConfig; virNetworkObjEndAPI; virNetworkObjFindByName; -virNetworkObjFindByNameLocked; virNetworkObjFindByUUID; -virNetworkObjFindByUUIDLocked; virNetworkObjGetPersistentDef; virNetworkObjListExport; virNetworkObjListForEach; -- 2.13.0
There is no reason why two threads trying to look up two networks should mutually exclude each other. Utilize new virObjectRWLockable that was just introduced. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virnetworkobj.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index d8eca1e90..79cbee5f3 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -38,7 +38,7 @@ VIR_LOG_INIT("conf.virnetworkobj"); #define CLASS_ID_BITMAP_SIZE (1<<16) struct _virNetworkObjList { - virObjectLockable parent; + virObjectRWLockable parent; virHashTablePtr objs; }; @@ -57,7 +57,7 @@ virNetworkObjOnceInit(void) virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(), "virNetworkObjList", sizeof(virNetworkObjList), virNetworkObjListDispose))) @@ -115,7 +115,7 @@ virNetworkObjListNew(void) if (virNetworkObjInitialize() < 0) return NULL; - if (!(nets = virObjectLockableNew(virNetworkObjListClass))) + if (!(nets = virObjectRWLockableNew(virNetworkObjListClass))) return NULL; if (!(nets->objs = virHashCreate(50, virObjectFreeHashData))) { @@ -159,7 +159,7 @@ virNetworkObjFindByUUID(virNetworkObjListPtr nets, { virNetworkObjPtr ret; - virObjectLock(nets); + virObjectLockRead(nets); ret = virNetworkObjFindByUUIDLocked(nets, uuid); virObjectUnlock(nets); if (ret) @@ -213,7 +213,7 @@ virNetworkObjFindByName(virNetworkObjListPtr nets, { virNetworkObjPtr ret; - virObjectLock(nets); + virObjectLockRead(nets); ret = virNetworkObjFindByNameLocked(nets, name); virObjectUnlock(nets); if (ret) @@ -961,7 +961,7 @@ virNetworkObjBridgeInUse(virNetworkObjListPtr nets, virNetworkObjPtr obj; struct virNetworkObjBridgeInUseHelperData data = {bridge, skipname}; - virObjectLock(nets); + virObjectLockRead(nets); obj = virHashSearch(nets->objs, virNetworkObjBridgeInUseHelper, &data, NULL); virObjectUnlock(nets); @@ -1148,7 +1148,7 @@ virNetworkObjListExport(virConnectPtr conn, int ret = -1; struct virNetworkObjListData data = { conn, NULL, filter, flags, 0, false}; - virObjectLock(netobjs); + virObjectLockRead(netobjs); if (nets && VIR_ALLOC_N(data.nets, virHashSize(netobjs->objs) + 1) < 0) goto cleanup; @@ -1213,7 +1213,7 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, void *opaque) { struct virNetworkObjListForEachHelperData data = {callback, opaque, 0}; - virObjectLock(nets); + virObjectLockRead(nets); virHashForEach(nets->objs, virNetworkObjListForEachHelper, &data); virObjectUnlock(nets); return data.ret; @@ -1280,7 +1280,7 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, struct virNetworkObjListGetHelperData data = { conn, filter, names, nnames, active, 0, false}; - virObjectLock(nets); + virObjectLockRead(nets); virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); virObjectUnlock(nets); @@ -1306,7 +1306,7 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, struct virNetworkObjListGetHelperData data = { conn, filter, NULL, -1, active, 0, false}; - virObjectLock(nets); + virObjectLockRead(nets); virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); virObjectUnlock(nets); -- 2.13.0
There is no reason why two threads trying to look up two node devices should mutually exclude each other. Utilize new virObjectRWLockable that was just introduced. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 3ab93a3ea..808365e21 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -40,7 +40,7 @@ struct _virNodeDeviceObj { }; struct _virNodeDeviceObjList { - virObjectLockable parent; + virObjectRWLockable parent; /* name string -> virNodeDeviceObj mapping * for O(1), lockless lookup-by-name */ @@ -63,7 +63,7 @@ virNodeDeviceObjOnceInit(void) virNodeDeviceObjDispose))) return -1; - if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectRWLockable(), "virNodeDeviceObjList", sizeof(virNodeDeviceObjList), virNodeDeviceObjListDispose))) @@ -231,7 +231,7 @@ virNodeDeviceObjListSearch(virNodeDeviceObjListPtr devs, { virNodeDeviceObjPtr obj; - virObjectLock(devs); + virObjectLockRead(devs); obj = virHashSearch(devs->objs, callback, data, NULL); virObjectRef(obj); virObjectUnlock(devs); @@ -284,7 +284,7 @@ virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, { virNodeDeviceObjPtr obj; - virObjectLock(devs); + virObjectLockRead(devs); obj = virNodeDeviceObjListFindByNameLocked(devs, name); virObjectUnlock(devs); if (obj) @@ -462,7 +462,7 @@ virNodeDeviceObjListNew(void) if (virNodeDeviceObjInitialize() < 0) return NULL; - if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass))) + if (!(devs = virObjectRWLockableNew(virNodeDeviceObjListClass))) return NULL; if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) { @@ -767,7 +767,7 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, struct virNodeDeviceCountData data = { .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .count = 0 }; - virObjectLock(devs); + virObjectLockRead(devs); virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data); virObjectUnlock(devs); @@ -828,7 +828,7 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .names = names, .nnames = 0, .maxnames = maxnames, .error = false }; - virObjectLock(devs); + virObjectLockRead(devs); virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data); virObjectUnlock(devs); @@ -932,7 +932,7 @@ virNodeDeviceObjListExport(virConnectPtr conn, .conn = conn, .aclfilter = aclfilter, .flags = flags, .devices = NULL, .ndevices = 0, .error = false }; - virObjectLock(devs); + virObjectLockRead(devs); if (devices && VIR_ALLOC_N(data.devices, virHashSize(devs->objs) + 1) < 0) { virObjectUnlock(devs); -- 2.13.0
There is no reason why two threads trying to copy error from connection should mutually exclude each other. Utilize new virObjectRWLockable that was just introduced. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/datatypes.c | 6 ++++-- src/datatypes.h | 2 +- src/util/virerror.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 46414ae29..c3e143c7e 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -83,8 +83,10 @@ virDataTypesOnceInit(void) DECLARE_CLASS_COMMON(basename, virClassForObject()) #define DECLARE_CLASS_LOCKABLE(basename) \ DECLARE_CLASS_COMMON(basename, virClassForObjectLockable()) +#define DECLARE_CLASS_RWLOCKABLE(basename) \ + DECLARE_CLASS_COMMON(basename, virClassForObjectRWLockable()) - DECLARE_CLASS_LOCKABLE(virConnect); + DECLARE_CLASS_RWLOCKABLE(virConnect); DECLARE_CLASS_LOCKABLE(virConnectCloseCallbackData); DECLARE_CLASS(virDomain); DECLARE_CLASS(virDomainSnapshot); @@ -124,7 +126,7 @@ virGetConnect(void) if (virDataTypesInitialize() < 0) return NULL; - return virObjectLockableNew(virConnectClass); + return virObjectRWLockableNew(virConnectClass); } /** diff --git a/src/datatypes.h b/src/datatypes.h index 8a0399cd0..7076c2b8d 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -453,7 +453,7 @@ struct _virAdmConnectCloseCallbackData { * Internal structure associated to a connection */ struct _virConnect { - virObjectLockable object; + virObjectRWLockable object; /* All the variables from here, until declared otherwise in one of * the following comments, are setup at time of connection open diff --git a/src/util/virerror.c b/src/util/virerror.c index ef17fb5e6..fead92d67 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -482,7 +482,7 @@ virConnCopyLastError(virConnectPtr conn, virErrorPtr to) if (conn == NULL) return -1; - virObjectLock(conn); + virObjectLockRead(conn); if (conn->err.code == VIR_ERR_OK) virResetError(to); else -- 2.13.0
On Thu, Jul 27, 2017 at 01:47:26PM +0200, Michal Privoznik wrote:
There is no reason why two threads trying to copy error from connection should mutually exclude each other. Utilize new virObjectRWLockable that was just introduced.
ACK, although I see it mostly as an example patch on how to adapt to RWLocks =)
Currently, the storage driver doesn't have virStoragePoolList object. It maintains the list itself. To mutually exclude two threads trying to access it mutexes are used. However, this hurts performance as there's no reason why two threads reading from the list cannot run at the same time. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 42 +++++++++++++++++++++++------------------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 5a61b2aa6..585c63869 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -53,7 +53,7 @@ typedef struct _virStorageDriverState virStorageDriverState; typedef virStorageDriverState *virStorageDriverStatePtr; struct _virStorageDriverState { - virMutex lock; + virRWLock lock; virStoragePoolObjList pools; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f162f2e5a..b2ba667ef 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -70,11 +70,15 @@ struct _virStorageVolStreamInfo { static void storageDriverLock(void) { - virMutexLock(&driver->lock); + virRWLockWrite(&driver->lock); +} +static void storageDriverLockRead(void) +{ + virRWLockRead(&driver->lock); } static void storageDriverUnlock(void) { - virMutexUnlock(&driver->lock); + virRWLockUnlock(&driver->lock); } @@ -253,7 +257,7 @@ storageStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) return ret; - if (virMutexInit(&driver->lock) < 0) { + if (virRWLockInit(&driver->lock) < 0) { virReportSystemError(errno, "%s", _("cannot initialize storage driver lock")); VIR_FREE(driver); @@ -382,7 +386,7 @@ storageStateCleanup(void) VIR_FREE(driver->autostartDir); VIR_FREE(driver->stateDir); storageDriverUnlock(); - virMutexDestroy(&driver->lock); + virRWLockDestroy(&driver->lock); VIR_FREE(driver); return 0; @@ -417,7 +421,7 @@ virStoragePoolObjFromStoragePool(virStoragePoolPtr pool) { virStoragePoolObjPtr ret; - storageDriverLock(); + storageDriverLockRead(); ret = storagePoolObjFindByUUID(pool->uuid, pool->name); storageDriverUnlock(); @@ -430,7 +434,7 @@ storagePoolObjFindByName(const char *name) { virStoragePoolObjPtr obj; - storageDriverLock(); + storageDriverLockRead(); if (!(obj = virStoragePoolObjFindByName(&driver->pools, name))) virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), name); @@ -447,7 +451,7 @@ storagePoolLookupByUUID(virConnectPtr conn, virStoragePoolObjPtr obj; virStoragePoolPtr pool = NULL; - storageDriverLock(); + storageDriverLockRead(); obj = storagePoolObjFindByUUID(uuid, NULL); storageDriverUnlock(); if (!obj) @@ -511,7 +515,7 @@ storageConnectNumOfStoragePools(virConnectPtr conn) if (virConnectNumOfStoragePoolsEnsureACL(conn) < 0) return -1; - storageDriverLock(); + storageDriverLockRead(); nactive = virStoragePoolObjNumOfStoragePools(&driver->pools, conn, true, virConnectNumOfStoragePoolsCheckACL); storageDriverUnlock(); @@ -530,7 +534,7 @@ storageConnectListStoragePools(virConnectPtr conn, if (virConnectListStoragePoolsEnsureACL(conn) < 0) return -1; - storageDriverLock(); + storageDriverLockRead(); got = virStoragePoolObjGetNames(&driver->pools, conn, true, virConnectListStoragePoolsCheckACL, names, maxnames); @@ -546,7 +550,7 @@ storageConnectNumOfDefinedStoragePools(virConnectPtr conn) if (virConnectNumOfDefinedStoragePoolsEnsureACL(conn) < 0) return -1; - storageDriverLock(); + storageDriverLockRead(); nactive = virStoragePoolObjNumOfStoragePools(&driver->pools, conn, false, virConnectNumOfDefinedStoragePoolsCheckACL); storageDriverUnlock(); @@ -565,7 +569,7 @@ storageConnectListDefinedStoragePools(virConnectPtr conn, if (virConnectListDefinedStoragePoolsEnsureACL(conn) < 0) return -1; - storageDriverLock(); + storageDriverLockRead(); got = virStoragePoolObjGetNames(&driver->pools, conn, false, virConnectListDefinedStoragePoolsCheckACL, names, maxnames); @@ -1457,7 +1461,7 @@ storageVolLookupByKey(virConnectPtr conn, size_t i; virStorageVolPtr vol = NULL; - storageDriverLock(); + storageDriverLockRead(); for (i = 0; i < driver->pools.count && !vol; i++) { virStoragePoolObjLock(driver->pools.objs[i]); if (virStoragePoolObjIsActive(driver->pools.objs[i])) { @@ -1502,7 +1506,7 @@ storageVolLookupByPath(virConnectPtr conn, if (!cleanpath) return NULL; - storageDriverLock(); + storageDriverLockRead(); for (i = 0; i < driver->pools.count && !vol; i++) { virStoragePoolObjPtr obj = driver->pools.objs[i]; virStorageVolDefPtr voldef; @@ -1597,7 +1601,7 @@ storagePoolLookupByTargetPath(virConnectPtr conn, if (!cleanpath) return NULL; - storageDriverLock(); + storageDriverLockRead(); for (i = 0; i < driver->pools.count && !pool; i++) { virStoragePoolObjPtr obj = driver->pools.objs[i]; @@ -1860,7 +1864,7 @@ storageVolCreateXML(virStoragePoolPtr pool, VIR_FREE(buildvoldef); - storageDriverLock(); + storageDriverLockRead(); virStoragePoolObjLock(obj); storageDriverUnlock(); @@ -1926,7 +1930,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, VIR_STORAGE_VOL_CREATE_REFLINK, NULL); - storageDriverLock(); + storageDriverLockRead(); obj = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid); if (obj && STRNEQ(pool->name, volsrc->pool)) { virStoragePoolObjUnlock(obj); @@ -2060,7 +2064,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, buildret = backend->buildVolFrom(pool->conn, obj, shadowvol, voldefsrc, flags); - storageDriverLock(); + storageDriverLockRead(); virStoragePoolObjLock(obj); if (objsrc) virStoragePoolObjLock(objsrc); @@ -2624,7 +2628,7 @@ storageConnectListAllStoragePools(virConnectPtr conn, if (virConnectListAllStoragePoolsEnsureACL(conn) < 0) goto cleanup; - storageDriverLock(); + storageDriverLockRead(); ret = virStoragePoolObjListExport(conn, &driver->pools, pools, virConnectListAllStoragePoolsCheckACL, flags); @@ -3029,7 +3033,7 @@ virStoragePoolObjFindPoolByUUID(const unsigned char *uuid) { virStoragePoolObjPtr obj; - storageDriverLock(); + storageDriverLockRead(); obj = virStoragePoolObjFindByUUID(&driver->pools, uuid); storageDriverUnlock(); return obj; -- 2.13.0
On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
As I started to turn more object into using RW locks, I've found couple of areas for improvement too.
Michal Privoznik (7): virConnect: Update comment for @privateData Report error if virMutexInit fails virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static again virNetworkObjList: Derive from virObjectRWLockable virNodeDeviceObjList: Derive from virObjectRWLockable virConnect: Derive from virObjectRWLockable storageDriver: Use RW locks
The patches I have not replied to look fine, but I think it would be easier to modify the common object after John's patches. Are any of those non-conflicting with those series? If yes, I can review those into more detail.
src/bhyve/bhyve_driver.c | 1 + src/conf/virnetworkobj.c | 42 ++++++++++---------------------- src/conf/virnetworkobj.h | 8 ------- src/conf/virnodedeviceobj.c | 16 ++++++------- src/conf/virstorageobj.h | 2 +- src/datatypes.c | 6 +++-- src/datatypes.h | 6 ++--- src/libvirt_private.syms | 2 -- src/lxc/lxc_driver.c | 1 + src/lxc/lxc_fuse.c | 4 +++- src/network/bridge_driver.c | 1 + src/node_device/node_device_hal.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 12 +++++++--- src/nwfilter/nwfilter_driver.c | 5 +++- src/nwfilter/nwfilter_gentech_driver.c | 4 +++- src/secret/secret_driver.c | 2 ++ src/storage/storage_driver.c | 44 +++++++++++++++++++--------------- src/uml/uml_driver.c | 1 + src/util/virerror.c | 2 +- src/util/virnetlink.c | 1 + src/util/virthreadpool.c | 4 +++- src/vmware/vmware_driver.c | 5 +++- src/vz/vz_driver.c | 4 +++- tools/virsh-console.c | 4 +++- 24 files changed, 94 insertions(+), 84 deletions(-)
-- 2.13.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/28/2017 10:32 AM, Martin Kletzander wrote:
On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
As I started to turn more object into using RW locks, I've found couple of areas for improvement too.
Michal Privoznik (7): virConnect: Update comment for @privateData Report error if virMutexInit fails virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static again virNetworkObjList: Derive from virObjectRWLockable virNodeDeviceObjList: Derive from virObjectRWLockable virConnect: Derive from virObjectRWLockable storageDriver: Use RW locks
The patches I have not replied to look fine, but I think it would be easier to modify the common object after John's patches. Are any of those non-conflicting with those series? If yes, I can review those into more detail.
I had contacted Michal via IRC about this when I saw these hit the list. I'd prefer to see them handled via a common object set of patches. However, that said... I wish the RWLockable hadn't just gone in so quickly, but what's done is done. I have a couple of other thoughts in this area: * I think virObjectLockableRead should return 0/-1 and have the caller handle it. * I think there should be a virObjectLockableWrite w/ same return value checking. * I think virObjectLock should not handle either RWLockable or Lockable. It should just handle Lockable. * There could be a virObjectRWUnlock, but virObjectUnlock should be "OK" to not be specific since theoretically one would have already locked and got something valid. I think through this common object series I've found a few instances where Unlock was called with an Unlocked object which is a different can-o-worms. I have not come across any instance where Unlock was called with NULL or invalid parameter. And if it was, the worse thing that could happen is we wouldn't unlock the resource and that'd be found relatively quickly by the next locker. Debugging it would be a beachball though. I do have some patches I put together which I'll post some time today with any luck... John FWIW: As noted in my responses to the RWLock series, consider if virObjectLock(obj) is called with an invalid @obj, then we really don't get the lock. All that's done is a VIR_WARN() and return. So if someone passes the wrong thing we have all sorts of problems. That's been true of virObjectLock for a long time, but we have (ahem) well behaved code so less of a problem. Introducing virObjectRWLockable gives us the opportunity to not only have concurrency in locks, but also to allow error checking which should have been done in virObjectLock, but there's way too many callers now to undo that and using abort() or something similar to that void function is considered bad form. Also IMO having a virObjectLockRead could lead someone to believe that passing a virObjectLockable object would work just fine. Which, well it would, but not really. Passing a virObjectLockable would cause a VIR_WARN message and return without the resource really locked, which isn't good. Since LockRead and Lock can be used in the same code path it means we need to be more careful when reviewing to know which passed argument was a RWLock style lock. Right now that's @doms or @domlist for RWLocks and @obj, @dom, and @vm for existing locks. Not look closely enough and you'll miss that @dom cannot be use for RWLock in the same code path that's using @doms that can be used that way. If the goal is to eventually convert to using RWLocks more, then take the plunge now to check the status or forever be shackled with the problem that virObjectLock has. When converting over a lock to be an RWLock that means any code that touches the lock will need to use either a Read or Write API, but to me that's goodness.
src/bhyve/bhyve_driver.c | 1 + src/conf/virnetworkobj.c | 42 ++++++++++---------------------- src/conf/virnetworkobj.h | 8 ------- src/conf/virnodedeviceobj.c | 16 ++++++------- src/conf/virstorageobj.h | 2 +- src/datatypes.c | 6 +++-- src/datatypes.h | 6 ++--- src/libvirt_private.syms | 2 -- src/lxc/lxc_driver.c | 1 + src/lxc/lxc_fuse.c | 4 +++- src/network/bridge_driver.c | 1 + src/node_device/node_device_hal.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 12 +++++++--- src/nwfilter/nwfilter_driver.c | 5 +++- src/nwfilter/nwfilter_gentech_driver.c | 4 +++- src/secret/secret_driver.c | 2 ++ src/storage/storage_driver.c | 44 +++++++++++++++++++--------------- src/uml/uml_driver.c | 1 + src/util/virerror.c | 2 +- src/util/virnetlink.c | 1 + src/util/virthreadpool.c | 4 +++- src/vmware/vmware_driver.c | 5 +++- src/vz/vz_driver.c | 4 +++- tools/virsh-console.c | 4 +++- 24 files changed, 94 insertions(+), 84 deletions(-)
-- 2.13.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
On 07/28/2017 10:32 AM, Martin Kletzander wrote:
On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
As I started to turn more object into using RW locks, I've found couple of areas for improvement too.
Michal Privoznik (7): virConnect: Update comment for @privateData Report error if virMutexInit fails virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static again virNetworkObjList: Derive from virObjectRWLockable virNodeDeviceObjList: Derive from virObjectRWLockable virConnect: Derive from virObjectRWLockable storageDriver: Use RW locks
The patches I have not replied to look fine, but I think it would be easier to modify the common object after John's patches. Are any of those non-conflicting with those series? If yes, I can review those into more detail.
I had contacted Michal via IRC about this when I saw these hit the list. I'd prefer to see them handled via a common object set of patches.
However, that said... I wish the RWLockable hadn't just gone in so quickly, but what's done is done. I have a couple of other thoughts in this area:
* I think virObjectLockableRead should return 0/-1 and have the caller handle it. * I think there should be a virObjectLockableWrite w/ same return value checking.
I rather disagree with that - it just adds a massive amount more code to deal with failures from the lock apis that should never happen unless you've already screwed up somewhere else in your code. If the object you've passed into the methods has already been freed, then you're already doomed and trying to recover from that is never going to be reliable - in fact it could cause more trouble. The memory for the object passed in is either in the free pool (and so shouldn't be touched at all), or has been reused and allocated for some other object now (and so again touching it is a bad idea). Trying to detect & handle these situatuons is just doomed to be racy or dangerous or both
* I think virObjectLock should not handle either RWLockable or Lockable. It should just handle Lockable.
I think that made sense as an intermediate step, to avoid having to bulk-convert all code at once, but agree that it would be reasonable to add a virObjectRWLockWrite() method, convert code over, and then finally remove the code stuff in virObjectLock
* There could be a virObjectRWUnlock, but virObjectUnlock should be "OK" to not be specific since theoretically one would have already locked and got something valid. I think through this common object series I've found a few instances where Unlock was called with an Unlocked object which is a different can-o-worms. I have not come across any instance where Unlock was called with NULL or invalid parameter. And if it was, the worse thing that could happen is we wouldn't unlock the resource and that'd be found relatively quickly by the next locker. Debugging it would be a beachball though.
I think it would make sense to have a virObjectRWUnlock too. Both of these changes would make it clearer which bit of code is using a plain lockable object, vs a rwlockable object.
FWIW: As noted in my responses to the RWLock series, consider if virObjectLock(obj) is called with an invalid @obj, then we really don't get the lock. All that's done is a VIR_WARN() and return. So if someone passes the wrong thing we have all sorts of problems. That's been true of virObjectLock for a long time, but we have (ahem) well behaved code so less of a problem.
As above, trying to detect these errors & carry on & cleanup is just giving a false sense of security. I think this is probably a case where it is reasonable to abort() immediately, because if you are in this messed up state, the chances are that the code is going to abort due to memory corrupton shortly anyway. 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 :|
On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
On 07/28/2017 10:32 AM, Martin Kletzander wrote:
On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
As I started to turn more object into using RW locks, I've found couple of areas for improvement too.
Michal Privoznik (7): virConnect: Update comment for @privateData Report error if virMutexInit fails virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static again virNetworkObjList: Derive from virObjectRWLockable virNodeDeviceObjList: Derive from virObjectRWLockable virConnect: Derive from virObjectRWLockable storageDriver: Use RW locks
The patches I have not replied to look fine, but I think it would be easier to modify the common object after John's patches. Are any of those non-conflicting with those series? If yes, I can review those into more detail.
I had contacted Michal via IRC about this when I saw these hit the list. I'd prefer to see them handled via a common object set of patches.
However, that said... I wish the RWLockable hadn't just gone in so quickly, but what's done is done. I have a couple of other thoughts in this area:
* I think virObjectLockableRead should return 0/-1 and have the caller handle it. * I think there should be a virObjectLockableWrite w/ same return value checking.
I rather disagree with that - it just adds a massive amount more code to deal with failures from the lock apis that should never happen unless you've already screwed up somewhere else in your code. If the object you've passed into the methods has already been freed, then you're already doomed and trying to recover from that is never going to be reliable - in fact it could cause more trouble. The memory for the object passed in is either in the free pool (and so shouldn't be touched at all), or has been reused and allocated for some other object now (and so again touching it is a bad idea). Trying to detect & handle these situatuons is just doomed to be racy or dangerous or both
I agree w/ the screw up part. Obviously for me it's the RW vs non-RW usage that sent me down this path... Still, I'm not sure what you mean by massive amount of code to deal with failures. I was considering only the RW lock mgmt. Currently only virdomainobjlist was modified to add virObjectLockRead and only done within the last week. There's 9 virObjectLockRead calls and would be 4 virObjectLockWrite calls. if (virObjectLock{Read|Write}(obj) < 0) {goto {cleanup|error}|return -1|return NULL}; The only place this doesn't work properly is the vir*Remove() calls which are void functions. We'd still be "stuck" with them. Within virObjectLock{Read|Write}() in the failure path: virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to obtain rwlock for object=%p"), anyobj); The code would return 0 on success and -1 on failure.
* I think virObjectLock should not handle either RWLockable or Lockable. It should just handle Lockable.
I think that made sense as an intermediate step, to avoid having to bulk-convert all code at once, but agree that it would be reasonable to add a virObjectRWLockWrite() method, convert code over, and then finally remove the code stuff in virObjectLock
* There could be a virObjectRWUnlock, but virObjectUnlock should be "OK" to not be specific since theoretically one would have already locked and got something valid. I think through this common object series I've found a few instances where Unlock was called with an Unlocked object which is a different can-o-worms. I have not come across any instance where Unlock was called with NULL or invalid parameter. And if it was, the worse thing that could happen is we wouldn't unlock the resource and that'd be found relatively quickly by the next locker. Debugging it would be a beachball though.
I think it would make sense to have a virObjectRWUnlock too.
Both of these changes would make it clearer which bit of code is using a plain lockable object, vs a rwlockable object.
That's fine - I'll add that to my patches before sending...
FWIW: As noted in my responses to the RWLock series, consider if virObjectLock(obj) is called with an invalid @obj, then we really don't get the lock. All that's done is a VIR_WARN() and return. So if someone passes the wrong thing we have all sorts of problems. That's been true of virObjectLock for a long time, but we have (ahem) well behaved code so less of a problem.
As above, trying to detect these errors & carry on & cleanup is just giving a false sense of security. I think this is probably a case where it is reasonable to abort() immediately, because if you are in this messed up state, the chances are that the code is going to abort due to memory corrupton shortly anyway.
Regards, Daniel
Well I can propose the abort() on error if so desired. I agree w/r/t some awful things that could happen... John
On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote:
On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
On 07/28/2017 10:32 AM, Martin Kletzander wrote:
On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
As I started to turn more object into using RW locks, I've found couple of areas for improvement too.
Michal Privoznik (7): virConnect: Update comment for @privateData Report error if virMutexInit fails virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static again virNetworkObjList: Derive from virObjectRWLockable virNodeDeviceObjList: Derive from virObjectRWLockable virConnect: Derive from virObjectRWLockable storageDriver: Use RW locks
The patches I have not replied to look fine, but I think it would be easier to modify the common object after John's patches. Are any of those non-conflicting with those series? If yes, I can review those into more detail.
I had contacted Michal via IRC about this when I saw these hit the list. I'd prefer to see them handled via a common object set of patches.
However, that said... I wish the RWLockable hadn't just gone in so quickly, but what's done is done. I have a couple of other thoughts in this area:
* I think virObjectLockableRead should return 0/-1 and have the caller handle it. * I think there should be a virObjectLockableWrite w/ same return value checking.
I rather disagree with that - it just adds a massive amount more code to deal with failures from the lock apis that should never happen unless you've already screwed up somewhere else in your code. If the object you've passed into the methods has already been freed, then you're already doomed and trying to recover from that is never going to be reliable - in fact it could cause more trouble. The memory for the object passed in is either in the free pool (and so shouldn't be touched at all), or has been reused and allocated for some other object now (and so again touching it is a bad idea). Trying to detect & handle these situatuons is just doomed to be racy or dangerous or both
I agree w/ the screw up part. Obviously for me it's the RW vs non-RW usage that sent me down this path...
Still, I'm not sure what you mean by massive amount of code to deal with failures. I was considering only the RW lock mgmt. Currently only virdomainobjlist was modified to add virObjectLockRead and only done within the last week. There's 9 virObjectLockRead calls and would be 4 virObjectLockWrite calls.
if (virObjectLock{Read|Write}(obj) < 0) {goto {cleanup|error}|return -1|return NULL};
That's probably buggy if you use existing goto's, because many of those cleanup/error locations will call virObjectUnlock(obj), so you'll need to introduce another set of gotoo labels to optionally skip the unlock step. This is why I think it makes the code more complex for dubious benefit.
The only place this doesn't work properly is the vir*Remove() calls which are void functions. We'd still be "stuck" with them.
Yes that's another scenario I imagined - there are case where it simply isn't practical to do cleanup when locking fails.
Well I can propose the abort() on error if so desired. I agree w/r/t some awful things that could happen...
If we separate virObjectLock vs virObjectRWLockWrite() then, we can just unconditionally reference the object in the virObjectLock method and just let the abort happen naturally, without needing explicit abort 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 :|
On Fri, Jul 28, 2017 at 04:58:57PM +0100, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote:
On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
On 07/28/2017 10:32 AM, Martin Kletzander wrote:
On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote:
As I started to turn more object into using RW locks, I've found couple of areas for improvement too.
Michal Privoznik (7): virConnect: Update comment for @privateData Report error if virMutexInit fails virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static again virNetworkObjList: Derive from virObjectRWLockable virNodeDeviceObjList: Derive from virObjectRWLockable virConnect: Derive from virObjectRWLockable storageDriver: Use RW locks
The patches I have not replied to look fine, but I think it would be easier to modify the common object after John's patches. Are any of those non-conflicting with those series? If yes, I can review those into more detail.
I had contacted Michal via IRC about this when I saw these hit the list. I'd prefer to see them handled via a common object set of patches.
However, that said... I wish the RWLockable hadn't just gone in so quickly, but what's done is done. I have a couple of other thoughts in this area:
* I think virObjectLockableRead should return 0/-1 and have the caller handle it. * I think there should be a virObjectLockableWrite w/ same return value checking.
I rather disagree with that - it just adds a massive amount more code to deal with failures from the lock apis that should never happen unless you've already screwed up somewhere else in your code. If the object you've passed into the methods has already been freed, then you're already doomed and trying to recover from that is never going to be reliable - in fact it could cause more trouble. The memory for the object passed in is either in the free pool (and so shouldn't be touched at all), or has been reused and allocated for some other object now (and so again touching it is a bad idea). Trying to detect & handle these situatuons is just doomed to be racy or dangerous or both
I agree w/ the screw up part. Obviously for me it's the RW vs non-RW usage that sent me down this path...
Still, I'm not sure what you mean by massive amount of code to deal with failures. I was considering only the RW lock mgmt. Currently only virdomainobjlist was modified to add virObjectLockRead and only done within the last week. There's 9 virObjectLockRead calls and would be 4 virObjectLockWrite calls.
if (virObjectLock{Read|Write}(obj) < 0) {goto {cleanup|error}|return -1|return NULL};
That's probably buggy if you use existing goto's, because many of those cleanup/error locations will call virObjectUnlock(obj), so you'll need to introduce another set of gotoo labels to optionally skip the unlock step. This is why I think it makes the code more complex for dubious benefit.
The only place this doesn't work properly is the vir*Remove() calls which are void functions. We'd still be "stuck" with them.
Yes that's another scenario I imagined - there are case where it simply isn't practical to do cleanup when locking fails.
Well I can propose the abort() on error if so desired. I agree w/r/t some awful things that could happen...
If we separate virObjectLock vs virObjectRWLockWrite() then, we can just unconditionally reference the object in the virObjectLock method and just let the abort happen naturally, without needing explicit abort
I agree with most of it, but I can't wrap my head around what you meant by this paragraph, could you explain it to someone whose brain is just not working yet, please?
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 :|
On Mon, Jul 31, 2017 at 08:58:41AM +0200, Martin Kletzander wrote:
On Fri, Jul 28, 2017 at 04:58:57PM +0100, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote:
On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
On 07/28/2017 10:32 AM, Martin Kletzander wrote:
On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote: > As I started to turn more object into using RW locks, I've found > couple of > areas for improvement too. > > Michal Privoznik (7): > virConnect: Update comment for @privateData > Report error if virMutexInit fails > virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static > again > virNetworkObjList: Derive from virObjectRWLockable > virNodeDeviceObjList: Derive from virObjectRWLockable > virConnect: Derive from virObjectRWLockable > storageDriver: Use RW locks >
The patches I have not replied to look fine, but I think it would be easier to modify the common object after John's patches. Are any of those non-conflicting with those series? If yes, I can review those into more detail.
I had contacted Michal via IRC about this when I saw these hit the list. I'd prefer to see them handled via a common object set of patches.
However, that said... I wish the RWLockable hadn't just gone in so quickly, but what's done is done. I have a couple of other thoughts in this area:
* I think virObjectLockableRead should return 0/-1 and have the caller handle it. * I think there should be a virObjectLockableWrite w/ same return value checking.
I rather disagree with that - it just adds a massive amount more code to deal with failures from the lock apis that should never happen unless you've already screwed up somewhere else in your code. If the object you've passed into the methods has already been freed, then you're already doomed and trying to recover from that is never going to be reliable - in fact it could cause more trouble. The memory for the object passed in is either in the free pool (and so shouldn't be touched at all), or has been reused and allocated for some other object now (and so again touching it is a bad idea). Trying to detect & handle these situatuons is just doomed to be racy or dangerous or both
I agree w/ the screw up part. Obviously for me it's the RW vs non-RW usage that sent me down this path...
Still, I'm not sure what you mean by massive amount of code to deal with failures. I was considering only the RW lock mgmt. Currently only virdomainobjlist was modified to add virObjectLockRead and only done within the last week. There's 9 virObjectLockRead calls and would be 4 virObjectLockWrite calls.
if (virObjectLock{Read|Write}(obj) < 0) {goto {cleanup|error}|return -1|return NULL};
That's probably buggy if you use existing goto's, because many of those cleanup/error locations will call virObjectUnlock(obj), so you'll need to introduce another set of gotoo labels to optionally skip the unlock step. This is why I think it makes the code more complex for dubious benefit.
The only place this doesn't work properly is the vir*Remove() calls which are void functions. We'd still be "stuck" with them.
Yes that's another scenario I imagined - there are case where it simply isn't practical to do cleanup when locking fails.
Well I can propose the abort() on error if so desired. I agree w/r/t some awful things that could happen...
If we separate virObjectLock vs virObjectRWLockWrite() then, we can just unconditionally reference the object in the virObjectLock method and just let the abort happen naturally, without needing explicit abort
I agree with most of it, but I can't wrap my head around what you meant by this paragraph, could you explain it to someone whose brain is just not working yet, please?
Currently we have: void virObjectLock(void *anyobj) { if (virObjectIsClass(anyobj, virObjectLockableClass)) { virObjectLockablePtr obj = anyobj; virMutexLock(&obj->lock); } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { virObjectRWLockablePtr obj = anyobj; virRWLockWrite(&obj->lock); } else { virObjectPtr obj = anyobj; VIR_WARN("Object %p (%s) is not a virObjectLockable " "nor virObjectRWLockable instance", anyobj, obj ? obj->klass->name : "(unknown)"); } } What I'm suggesting is void virObjectLock(void *anyobj) { virObjectLockablePtr obj = anyobj; virMutexLock(&obj->lock); } void virObjectRWLock(void *anyobj) { virObjectRWLockablePtr obj = anyobj; virRWLockWrite(&obj->lock); } eg just assume the caller has written code correctly and passing the right type of object. 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 :|
On Mon, Jul 31, 2017 at 10:26:30AM +0100, Daniel P. Berrange wrote:
On Mon, Jul 31, 2017 at 08:58:41AM +0200, Martin Kletzander wrote:
On Fri, Jul 28, 2017 at 04:58:57PM +0100, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:47:56AM -0400, John Ferlan wrote:
On 07/28/2017 11:24 AM, Daniel P. Berrange wrote:
On Fri, Jul 28, 2017 at 11:09:03AM -0400, John Ferlan wrote:
On 07/28/2017 10:32 AM, Martin Kletzander wrote: > On Thu, Jul 27, 2017 at 01:47:20PM +0200, Michal Privoznik wrote: >> As I started to turn more object into using RW locks, I've found >> couple of >> areas for improvement too. >> >> Michal Privoznik (7): >> virConnect: Update comment for @privateData >> Report error if virMutexInit fails >> virnetworkobj: Make virNetworkObjFindBy{UUID,Name}Locked() static >> again >> virNetworkObjList: Derive from virObjectRWLockable >> virNodeDeviceObjList: Derive from virObjectRWLockable >> virConnect: Derive from virObjectRWLockable >> storageDriver: Use RW locks >> > > The patches I have not replied to look fine, but I think it would be > easier to modify the common object after John's patches. Are any of > those non-conflicting with those series? If yes, I can review those > into more detail. >
I had contacted Michal via IRC about this when I saw these hit the list. I'd prefer to see them handled via a common object set of patches.
However, that said... I wish the RWLockable hadn't just gone in so quickly, but what's done is done. I have a couple of other thoughts in this area:
* I think virObjectLockableRead should return 0/-1 and have the caller handle it. * I think there should be a virObjectLockableWrite w/ same return value checking.
I rather disagree with that - it just adds a massive amount more code to deal with failures from the lock apis that should never happen unless you've already screwed up somewhere else in your code. If the object you've passed into the methods has already been freed, then you're already doomed and trying to recover from that is never going to be reliable - in fact it could cause more trouble. The memory for the object passed in is either in the free pool (and so shouldn't be touched at all), or has been reused and allocated for some other object now (and so again touching it is a bad idea). Trying to detect & handle these situatuons is just doomed to be racy or dangerous or both
I agree w/ the screw up part. Obviously for me it's the RW vs non-RW usage that sent me down this path...
Still, I'm not sure what you mean by massive amount of code to deal with failures. I was considering only the RW lock mgmt. Currently only virdomainobjlist was modified to add virObjectLockRead and only done within the last week. There's 9 virObjectLockRead calls and would be 4 virObjectLockWrite calls.
if (virObjectLock{Read|Write}(obj) < 0) {goto {cleanup|error}|return -1|return NULL};
That's probably buggy if you use existing goto's, because many of those cleanup/error locations will call virObjectUnlock(obj), so you'll need to introduce another set of gotoo labels to optionally skip the unlock step. This is why I think it makes the code more complex for dubious benefit.
The only place this doesn't work properly is the vir*Remove() calls which are void functions. We'd still be "stuck" with them.
Yes that's another scenario I imagined - there are case where it simply isn't practical to do cleanup when locking fails.
Well I can propose the abort() on error if so desired. I agree w/r/t some awful things that could happen...
If we separate virObjectLock vs virObjectRWLockWrite() then, we can just unconditionally reference the object in the virObjectLock method and just let the abort happen naturally, without needing explicit abort
I agree with most of it, but I can't wrap my head around what you meant by this paragraph, could you explain it to someone whose brain is just not working yet, please?
Currently we have:
void virObjectLock(void *anyobj) { if (virObjectIsClass(anyobj, virObjectLockableClass)) { virObjectLockablePtr obj = anyobj; virMutexLock(&obj->lock); } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { virObjectRWLockablePtr obj = anyobj; virRWLockWrite(&obj->lock); } else { virObjectPtr obj = anyobj; VIR_WARN("Object %p (%s) is not a virObjectLockable " "nor virObjectRWLockable instance", anyobj, obj ? obj->klass->name : "(unknown)"); } }
What I'm suggesting is
void virObjectLock(void *anyobj) { virObjectLockablePtr obj = anyobj; virMutexLock(&obj->lock); }
void virObjectRWLock(void *anyobj) { virObjectRWLockablePtr obj = anyobj; virRWLockWrite(&obj->lock); }
eg just assume the caller has written code correctly and passing the right type of object.
So no error checking, not aborts, nothing. I liked the possibility of gradual changes from Mutexes to RWLocks when Lock() handled both. I understand we don't want to have any abort()s in our code, but I'm not really sure for this one. I also think we're missing lot of error handling in virthread (merely due to multiple implementations in the past?). Anyway, there will always be room for improvement.
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 :|
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Martin Kletzander -
Michal Privoznik