[libvirt] [PATCH v2 00/24] Drop network driver lock
Yet another version. This time with: 1) Peter's review worked in 2) Even more patches, that allow even more parallelism. With my test program [1], I've been able to go from 56s to 23s. 1: https://www.redhat.com/archives/libvir-list/2015-February/msg01214.html Michal Privoznik (24): network_conf: Introduce virNetworkObjListForEach network_conf: Introduce virNetworkObjListGetNames network_conf: Introduce virNetworkObjListNumOfNetworks network_conf: Introduce virNetworkObjListPrune bridge_driver: Adapt to new virNetworkObjList accessors test_driver: Adapt to new virNetworkObjList accessors parallels_network: Adapt to new virNetworkObjList accessors network_conf: Turn virNetworkObjList into virObject network_conf: Turn struct _virNetworkObjList private network_conf: Make virNetworkObj actually virObject virNetworkObjList: Derive from virObjectLockableClass network_conf: Introduce virNetworkObjEndAPI bridge_driver: Use virNetworkObjEndAPI test_driver: Use virNetworkObjEndAPI parallels_network: Use virNetworkObjEndAPI network_conf: Introduce locked versions of lookup functions virNetworkObjListPtr: Make APIs self-locking virNetworkObjFindBy*: Return an reference to found object bridge_driver: Drop networkDriverLock() from almost everywhere test_driver: Drop testDriverLock() from almost everywhere parallels_network: Drop parallelsDriverLock() from everywhere. bridge_driver: Use more of networkObjFromNetwork virNetworkObjUnsetDefTransient: Lock object list if needed virNetworkObjFindBy*: Don't lock all networks in the list cfg.mk | 3 - src/conf/network_conf.c | 422 +++++++++++++++++++++++++------- src/conf/network_conf.h | 52 ++-- src/libvirt_private.syms | 13 +- src/network/bridge_driver.c | 500 ++++++++++++++------------------------ src/parallels/parallels_network.c | 135 +++------- src/test/test_driver.c | 185 +++----------- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 634 insertions(+), 682 deletions(-) -- 2.0.5
This API will be used in the future to call passed callback over each network object in the list. It's slightly different to its virDomainObjListForEach counterpart, because virDomainObjList uses a hash table to store domain object, while virNetworkObjList uses an array. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 27 +++++++++++++++++++++++++++ src/conf/network_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 34 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d0e7e1b..cb54e56 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4290,3 +4290,30 @@ virNetworkObjListExport(virConnectPtr conn, VIR_FREE(tmp_nets); return ret; } + +/** + * virNetworkObjListForEach: + * @nets: a list of network objects + * @callback: function to call over each of object in the list + * @opaque: pointer to pass to the @callback + * + * Function iterates over the list of network objects and calls + * passed callback over each one of them. + * + * Returns: 0 on success, -1 otherwise. + */ +int +virNetworkObjListForEach(virNetworkObjListPtr nets, + virNetworkObjListIterator callback, + void *opaque) +{ + int ret = 0; + size_t i = 0; + + for (i = 0; i < nets->count; i++) { + if (callback(nets->objs[i], opaque) < 0) + ret = -1; + } + + return ret; +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 16aea99..749c7fb 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -445,6 +445,12 @@ int virNetworkObjListExport(virConnectPtr conn, virNetworkObjListFilter filter, unsigned int flags); +typedef int (*virNetworkObjListIterator)(virNetworkObjPtr net, + void *opaque); + +int virNetworkObjListForEach(virNetworkObjListPtr nets, + virNetworkObjListIterator callback, + void *opaque); /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13e0931..a7285d8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -569,6 +569,7 @@ virNetworkObjFree; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; +virNetworkObjListForEach; virNetworkObjListFree; virNetworkObjLock; virNetworkObjReplacePersistentDef; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:02 +0100, Michal Privoznik wrote:
This API will be used in the future to call passed callback over each network object in the list. It's slightly different to its virDomainObjListForEach counterpart, because virDomainObjList uses a hash table to store domain object, while virNetworkObjList uses an array.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 27 +++++++++++++++++++++++++++ src/conf/network_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 34 insertions(+)
ACK, Peter
An accessor following pattern laid out by virDomainObjList* APIs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 8 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 47 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index cb54e56..fdf5907 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4317,3 +4317,41 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, return ret; } + +int +virNetworkObjListGetNames(virNetworkObjListPtr nets, + bool active, + char **names, + int nnames, + virNetworkObjListFilter filter, + virConnectPtr conn) +{ + int got = 0; + size_t i; + + for (i = 0; i < nets->count && got < nnames; i++) { + virNetworkObjPtr obj = nets->objs[i]; + virNetworkObjLock(obj); + if (filter && !filter(conn, obj->def)) { + virNetworkObjUnlock(obj); + continue; + } + + if ((active && virNetworkObjIsActive(obj)) || + (!active && !virNetworkObjIsActive(obj))) { + if (VIR_STRDUP(names[got], obj->def->name) < 0) { + virNetworkObjUnlock(obj); + goto error; + } + got++; + } + virNetworkObjUnlock(obj); + } + + return got; + + error: + for (i = 0; i < got; i++) + VIR_FREE(names[i]); + return -1; +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 749c7fb..598ddc2 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -451,6 +451,14 @@ typedef int (*virNetworkObjListIterator)(virNetworkObjPtr net, int virNetworkObjListForEach(virNetworkObjListPtr nets, virNetworkObjListIterator callback, void *opaque); + +int virNetworkObjListGetNames(virNetworkObjListPtr nets, + bool active, + char **names, + int nnames, + virNetworkObjListFilter filter, + virConnectPtr conn); + /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7285d8..f572592 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -571,6 +571,7 @@ virNetworkObjIsDuplicate; virNetworkObjListExport; virNetworkObjListForEach; virNetworkObjListFree; +virNetworkObjListGetNames; virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:03 +0100, Michal Privoznik wrote:
An accessor following pattern laid out by virDomainObjList* APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 8 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 47 insertions(+)
ACK, Peter
An accessor following pattern laid out by virDomainObjList* APIs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 26 ++++++++++++++++++++++++++ src/conf/network_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 32 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index fdf5907..dea180a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4355,3 +4355,29 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, VIR_FREE(names[i]); return -1; } + +int +virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, + bool active, + virNetworkObjListFilter filter, + virConnectPtr conn) +{ + int count = 0; + size_t i; + + for (i = 0; i < nets->count; i++) { + virNetworkObjPtr obj = nets->objs[i]; + virNetworkObjLock(obj); + if (filter && !filter(conn, obj->def)) { + virNetworkObjUnlock(obj); + continue; + } + + if ((active && virNetworkObjIsActive(obj)) || + (!active && !virNetworkObjIsActive(obj))) + count++; + virNetworkObjUnlock(obj); + } + + return count; +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 598ddc2..bd9e3b4 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -459,6 +459,11 @@ int virNetworkObjListGetNames(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn); +int virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, + bool active, + virNetworkObjListFilter filter, + virConnectPtr conn); + /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f572592..57c1a8c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -572,6 +572,7 @@ virNetworkObjListExport; virNetworkObjListForEach; virNetworkObjListFree; virNetworkObjListGetNames; +virNetworkObjListNumOfNetworks; virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:04 +0100, Michal Privoznik wrote:
An accessor following pattern laid out by virDomainObjList* APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
ACK, Peter
The API will iterate over the list of network object and remove desired ones from it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 31 +++++++++++++++++++++++++++++++ src/conf/network_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 35 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index dea180a..ea9a9d4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4381,3 +4381,34 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, return count; } + +/** + * virNetworkObjListPrune: + * @nets: a list of network objects + * @flags: bitwise-OR of virConnectListAllNetworksFlags + * + * Iterate over list of network objects and remove the desired + * ones from it. + */ +void +virNetworkObjListPrune(virNetworkObjListPtr nets, + unsigned int flags) +{ + size_t i = 0; + + while (i < nets->count) { + virNetworkObjPtr obj = nets->objs[i]; + + virNetworkObjLock(obj); + + if (virNetworkMatch(obj, flags)) { + virNetworkObjUnlock(obj); + virNetworkObjFree(obj); + + VIR_DELETE_ELEMENT(nets->objs, i, nets->count); + } else { + virNetworkObjUnlock(obj); + i++; + } + } +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index bd9e3b4..3fbd609 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -464,6 +464,9 @@ int virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn); +void virNetworkObjListPrune(virNetworkObjListPtr nets, + unsigned int flags); + /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 57c1a8c..4ce5e3a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -573,6 +573,7 @@ virNetworkObjListForEach; virNetworkObjListFree; virNetworkObjListGetNames; virNetworkObjListNumOfNetworks; +virNetworkObjListPrune; virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:05 +0100, Michal Privoznik wrote:
The API will iterate over the list of network object and remove desired ones from it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 31 +++++++++++++++++++++++++++++++ src/conf/network_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 35 insertions(+)
ACK, Peter
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 334 ++++++++++++++++++++------------------------ 1 file changed, 148 insertions(+), 186 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2a61991..1878833 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -342,105 +342,87 @@ networkBridgeDummyNicName(const char *brname) return nicname; } -/* Update the internal status of all allegedly active networks - * according to external conditions on the host (i.e. anything that - * isn't stored directly in each network's state file). */ -static void -networkUpdateAllState(void) +static int +networkUpdateState(virNetworkObjPtr obj, + void *opaque ATTRIBUTE_UNUSED) { - size_t i; + int ret = -1; - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; + virNetworkObjLock(obj); + if (!virNetworkObjIsActive(obj)) { + virNetworkObjUnlock(obj); + return 0; + } - virNetworkObjLock(obj); - if (!virNetworkObjIsActive(obj)) { - virNetworkObjUnlock(obj); - continue; - } + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge doesn't exist, then mark it inactive */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + obj->active = 0; + break; - switch (obj->def->forward.type) { - case VIR_NETWORK_FORWARD_NONE: - case VIR_NETWORK_FORWARD_NAT: - case VIR_NETWORK_FORWARD_ROUTE: - /* If bridge doesn't exist, then mark it inactive */ - if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + case VIR_NETWORK_FORWARD_BRIDGE: + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) { obj->active = 0; break; - - case VIR_NETWORK_FORWARD_BRIDGE: - if (obj->def->bridge) { - if (virNetDevExists(obj->def->bridge) != 1) - obj->active = 0; - break; - } - /* intentionally drop through to common case for all - * macvtap networks (forward='bridge' with no bridge - * device defined is macvtap using its 'bridge' mode) - */ - case VIR_NETWORK_FORWARD_PRIVATE: - case VIR_NETWORK_FORWARD_VEPA: - case VIR_NETWORK_FORWARD_PASSTHROUGH: - /* so far no extra checks */ - break; - - case VIR_NETWORK_FORWARD_HOSTDEV: - /* so far no extra checks */ - break; - } - - /* Try and read dnsmasq/radvd pids of active networks */ - if (obj->active && obj->def->ips && (obj->def->nips > 0)) { - char *radvdpidbase; - - ignore_value(virPidFileReadIfAlive(driver->pidDir, - obj->def->name, - &obj->dnsmasqPid, - dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - radvdpidbase = networkRadvdPidfileBasename(obj->def->name); - if (!radvdpidbase) - break; - ignore_value(virPidFileReadIfAlive(driver->pidDir, - radvdpidbase, - &obj->radvdPid, RADVD)); - VIR_FREE(radvdpidbase); } - - virNetworkObjUnlock(obj); + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + /* so far no extra checks */ + break; + + case VIR_NETWORK_FORWARD_HOSTDEV: + /* so far no extra checks */ + break; } - /* remove inactive transient networks */ - i = 0; - while (i < driver->networks->count) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - - if (!obj->persistent && !obj->active) { - networkRemoveInactive(obj); - continue; - } - - virNetworkObjUnlock(obj); - i++; + /* Try and read dnsmasq/radvd pids of active networks */ + if (obj->active && obj->def->ips && (obj->def->nips > 0)) { + char *radvdpidbase; + + ignore_value(virPidFileReadIfAlive(driver->pidDir, + obj->def->name, + &obj->dnsmasqPid, + dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + goto cleanup; + + ignore_value(virPidFileReadIfAlive(driver->pidDir, + radvdpidbase, + &obj->radvdPid, RADVD)); + VIR_FREE(radvdpidbase); } + + ret = 0; + cleanup: + virNetworkObjUnlock(obj); + return ret; } - -static void -networkAutostartConfigs(void) +static int +networkAutostartConfig(virNetworkObjPtr net, + void *opaque ATTRIBUTE_UNUSED) { - size_t i; + int ret = -1; - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjLock(driver->networks->objs[i]); - if (driver->networks->objs[i]->autostart && - !virNetworkObjIsActive(driver->networks->objs[i])) { - if (networkStartNetwork(driver->networks->objs[i]) < 0) { - /* failed to start but already logged */ - } - } - virNetworkObjUnlock(driver->networks->objs[i]); - } + virNetworkObjLock(net); + if (net->autostart && + !virNetworkObjIsActive(net) && + networkStartNetwork(net) < 0) + goto cleanup; + + ret = 0; + cleanup: + virNetworkObjUnlock(net); + return ret; } #if HAVE_FIREWALLD @@ -650,7 +632,17 @@ networkStateInitialize(bool privileged, driver->networkAutostartDir) < 0) goto error; - networkUpdateAllState(); + + /* Update the internal status of all allegedly active + * networks according to external conditions on the host + * (i.e. anything that isn't stored directly in each + * network's state file). */ + virNetworkObjListForEach(driver->networks, + networkUpdateState, + NULL); + virNetworkObjListPrune(driver->networks, + VIR_CONNECT_LIST_NETWORKS_INACTIVE | + VIR_CONNECT_LIST_NETWORKS_TRANSIENT); networkReloadFirewallRules(); networkRefreshDaemons(); @@ -709,7 +701,9 @@ networkStateAutoStart(void) return; networkDriverLock(); - networkAutostartConfigs(); + virNetworkObjListForEach(driver->networks, + networkAutostartConfig, + NULL); networkDriverUnlock(); } @@ -733,7 +727,9 @@ networkStateReload(void) driver->networkAutostartDir); networkReloadFirewallRules(); networkRefreshDaemons(); - networkAutostartConfigs(); + virNetworkObjListForEach(driver->networks, + networkAutostartConfig, + NULL); networkDriverUnlock(); return 0; } @@ -1745,62 +1741,70 @@ networkRestartRadvd(virNetworkObjPtr network) } #endif /* #if 0 */ +static int +networkRefreshDaemonsHelper(virNetworkObjPtr net, + void *opaque ATTRIBUTE_UNUSED) +{ + + virNetworkObjLock(net); + if (virNetworkObjIsActive(net) && + ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || + (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || + (net->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + /* Only the three L3 network types that are configured by + * libvirt will have a dnsmasq or radvd daemon associated + * with them. Here we send a SIGHUP to an existing + * dnsmasq and/or radvd, or restart them if they've + * disappeared. + */ + networkRefreshDhcpDaemon(net); + networkRefreshRadvd(net); + } + virNetworkObjUnlock(net); + return 0; +} + /* SIGHUP/restart any dnsmasq or radvd daemons. * This should be called when libvirtd is restarted. */ static void networkRefreshDaemons(void) { - size_t i; - VIR_INFO("Refreshing network daemons"); + virNetworkObjListForEach(driver->networks, + networkRefreshDaemonsHelper, + NULL); +} - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr network = driver->networks->objs[i]; +static int +networkReloadFirewallRulesHelper(virNetworkObjPtr net, + void *opaque ATTRIBUTE_UNUSED) +{ - virNetworkObjLock(network); - if (virNetworkObjIsActive(network) && - ((network->def->forward.type == VIR_NETWORK_FORWARD_NONE) || - (network->def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { - /* Only the three L3 network types that are configured by - * libvirt will have a dnsmasq or radvd daemon associated - * with them. Here we send a SIGHUP to an existing - * dnsmasq and/or radvd, or restart them if they've - * disappeared. - */ - networkRefreshDhcpDaemon(network); - networkRefreshRadvd(network); + virNetworkObjLock(net); + if (virNetworkObjIsActive(net) && + ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || + (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || + (net->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + /* Only the three L3 network types that are configured by libvirt + * need to have iptables rules reloaded. + */ + networkRemoveFirewallRules(net->def); + if (networkAddFirewallRules(net->def) < 0) { + /* failed to add but already logged */ } - virNetworkObjUnlock(network); } + virNetworkObjUnlock(net); + return 0; } static void networkReloadFirewallRules(void) { - size_t i; - VIR_INFO("Reloading iptables rules"); - - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr network = driver->networks->objs[i]; - - virNetworkObjLock(network); - if (virNetworkObjIsActive(network) && - ((network->def->forward.type == VIR_NETWORK_FORWARD_NONE) || - (network->def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { - /* Only the three L3 network types that are configured by libvirt - * need to have iptables rules reloaded. - */ - networkRemoveFirewallRules(network->def); - if (networkAddFirewallRules(network->def) < 0) { - /* failed to add but already logged */ - } - } - virNetworkObjUnlock(network); - } + virNetworkObjListForEach(driver->networks, + networkReloadFirewallRulesHelper, + NULL); } /* Enable IP Forwarding. Return 0 for success, -1 for failure. */ @@ -2526,21 +2530,16 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, static int networkConnectNumOfNetworks(virConnectPtr conn) { - int nactive = 0; - size_t i; + int nactive; if (virConnectNumOfNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectNumOfNetworksCheckACL(conn, obj->def) && - virNetworkObjIsActive(obj)) - nactive++; - virNetworkObjUnlock(obj); - } + nactive = virNetworkObjListNumOfNetworks(driver->networks, + true, + virConnectNumOfNetworksCheckACL, + conn); networkDriverUnlock(); return nactive; @@ -2548,53 +2547,32 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) static int networkConnectListNetworks(virConnectPtr conn, char **const names, int nnames) { int got = 0; - size_t i; if (virConnectListNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count && got < nnames; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectListNetworksCheckACL(conn, obj->def) && - virNetworkObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virNetworkObjUnlock(obj); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(obj); - } + got = virNetworkObjListGetNames(driver->networks, + true, names, nnames, + virConnectListNetworksCheckACL, + conn); networkDriverUnlock(); return got; - - cleanup: - networkDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) { int ninactive = 0; - size_t i; if (virConnectNumOfDefinedNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectNumOfDefinedNetworksCheckACL(conn, obj->def) && - !virNetworkObjIsActive(obj)) - ninactive++; - virNetworkObjUnlock(obj); - } + ninactive = virNetworkObjListNumOfNetworks(driver->networks, + false, + virConnectNumOfDefinedNetworksCheckACL, + conn); networkDriverUnlock(); return ninactive; @@ -2602,33 +2580,17 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) static int networkConnectListDefinedNetworks(virConnectPtr conn, char **const names, int nnames) { int got = 0; - size_t i; if (virConnectListDefinedNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count && got < nnames; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectListDefinedNetworksCheckACL(conn, obj->def) && - !virNetworkObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virNetworkObjUnlock(obj); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(obj); - } + got = virNetworkObjListGetNames(driver->networks, + false, names, nnames, + virConnectListDefinedNetworksCheckACL, + conn); networkDriverUnlock(); return got; - - cleanup: - networkDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:06 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 334 ++++++++++++++++++++------------------------ 1 file changed, 148 insertions(+), 186 deletions(-)
ACK, Peter
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 64 ++++++++++---------------------------------------- 1 file changed, 12 insertions(+), 52 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 9591b7c..2bfe0ad 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3543,16 +3543,11 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, static int testConnectNumOfNetworks(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - int numActive = 0; - size_t i; + int numActive; testDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i])) - numActive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + numActive = virNetworkObjListNumOfNetworks(privconn->networks, + true, NULL, conn); testDriverUnlock(privconn); return numActive; @@ -3560,44 +3555,24 @@ static int testConnectNumOfNetworks(virConnectPtr conn) static int testConnectListNetworks(virConnectPtr conn, char **const names, int nnames) { testConnPtr privconn = conn->privateData; - int n = 0; - size_t i; + int n; testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->networks->count && n < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i]) && - VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto error; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + n = virNetworkObjListGetNames(privconn->networks, + true, names, nnames, NULL, conn); testDriverUnlock(privconn); return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; } static int testConnectNumOfDefinedNetworks(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - int numInactive = 0; - size_t i; + int numInactive; testDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i])) - numInactive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + numInactive = virNetworkObjListNumOfNetworks(privconn->networks, + false, NULL, conn); testDriverUnlock(privconn); return numInactive; @@ -3605,29 +3580,14 @@ static int testConnectNumOfDefinedNetworks(virConnectPtr conn) static int testConnectListDefinedNetworks(virConnectPtr conn, char **const names, int nnames) { testConnPtr privconn = conn->privateData; - int n = 0; - size_t i; + int n; testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->networks->count && n < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i]) && - VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto error; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + n = virNetworkObjListGetNames(privconn->networks, + false, names, nnames, NULL, conn); testDriverUnlock(privconn); return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; } static int -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:07 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 64 ++++++++++---------------------------------------- 1 file changed, 12 insertions(+), 52 deletions(-)
ACK, Peter
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 66 +++++++-------------------------------- 1 file changed, 12 insertions(+), 54 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index e1c6040..6b53518 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -357,17 +357,12 @@ int parallelsNetworkClose(virConnectPtr conn) static int parallelsConnectNumOfNetworks(virConnectPtr conn) { - int nactive = 0; - size_t i; + int nactive; parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i])) - nactive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + nactive = virNetworkObjListNumOfNetworks(privconn->networks, + true, NULL, conn); parallelsDriverUnlock(privconn); return nactive; @@ -378,45 +373,24 @@ static int parallelsConnectListNetworks(virConnectPtr conn, int nnames) { parallelsConnPtr privconn = conn->privateData; - int got = 0; - size_t i; + int got; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count && got < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i])) { - if (VIR_STRDUP(names[got], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + got = virNetworkObjListGetNames(privconn->networks, + true, names, nnames, NULL, conn); parallelsDriverUnlock(privconn); return got; - - cleanup: - parallelsDriverUnlock(privconn); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int parallelsConnectNumOfDefinedNetworks(virConnectPtr conn) { - int ninactive = 0; - size_t i; + int ninactive; parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i])) - ninactive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + ninactive = virNetworkObjListNumOfNetworks(privconn->networks, + false, NULL, conn); parallelsDriverUnlock(privconn); return ninactive; @@ -427,29 +401,13 @@ static int parallelsConnectListDefinedNetworks(virConnectPtr conn, int nnames) { parallelsConnPtr privconn = conn->privateData; - int got = 0; - size_t i; + int got; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count && got < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i])) { - if (VIR_STRDUP(names[got], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + got = virNetworkObjListGetNames(privconn->networks, + false, names, nnames, NULL, conn); parallelsDriverUnlock(privconn); return got; - - cleanup: - parallelsDriverUnlock(privconn); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int parallelsConnectListAllNetworks(virConnectPtr conn, -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:08 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 66 +++++++-------------------------------- 1 file changed, 12 insertions(+), 54 deletions(-)
ACK Peter
Well, one day this will be self-locking object, but not today. But lets prepare the code for that! Moreover, virNetworkObjListFree() is no longer needed, so turn it into virNetworkObjListDispose(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 1 - src/conf/network_conf.c | 53 +++++++++++++++++++++++++++++---------- src/conf/network_conf.h | 11 ++++---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 5 ++-- src/parallels/parallels_network.c | 7 +++--- src/test/test_driver.c | 13 ++++------ 7 files changed, 57 insertions(+), 35 deletions(-) diff --git a/cfg.mk b/cfg.mk index d72b039..07a794a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -250,7 +250,6 @@ useless_free_options = \ # n virNetworkFree (returns int) # n virNetworkFreeName (returns int) # y virNetworkObjFree -# n virNetworkObjListFree FIXME # n virNodeDevCapsDefFree FIXME # y virNodeDeviceDefFree # n virNodeDeviceFree (returns int) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ea9a9d4..f843e3c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -73,17 +73,33 @@ VIR_ENUM_IMPL(virNetworkForwardDriverName, VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, "hook-script"); -bool -virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint) +static virClassPtr virNetworkObjListClass; +static void virNetworkObjListDispose(void *obj); + +static int virNetworkObjOnceInit(void) +{ + if (!(virNetworkObjListClass = virClassNew(virClassForObject(), + "virNetworkObjList", + sizeof(virNetworkObjList), + virNetworkObjListDispose))) + return -1; + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virNetworkObj) + +virNetworkObjListPtr virNetworkObjListNew(void) { - unsigned int flag = (1 << taint); + virNetworkObjListPtr nets; + + if (virNetworkObjInitialize() < 0) + return NULL; - if (obj->taint & flag) - return false; + if (!(nets = virObjectNew(virNetworkObjListClass))) + return NULL; - obj->taint |= flag; - return true; + return nets; } virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, @@ -116,6 +132,19 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, return NULL; } +bool +virNetworkObjTaint(virNetworkObjPtr obj, + virNetworkTaintFlags taint) +{ + unsigned int flag = (1 << taint); + + if (obj->taint & flag) + return false; + + obj->taint |= flag; + return true; +} + static void virPortGroupDefClear(virPortGroupDefPtr def) @@ -275,18 +304,16 @@ void virNetworkObjFree(virNetworkObjPtr net) VIR_FREE(net); } -void virNetworkObjListFree(virNetworkObjListPtr nets) +static void +virNetworkObjListDispose(void *obj) { + virNetworkObjListPtr nets = obj; size_t i; - if (!nets) - return; - for (i = 0; i < nets->count; i++) virNetworkObjFree(nets->objs[i]); VIR_FREE(nets->objs); - nets->count = 0; } /* diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3fbd609..0c2609a 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -40,6 +40,7 @@ # include "device_conf.h" # include "virbitmap.h" # include "networkcommon_conf.h" +# include "virobject.h" typedef enum { VIR_NETWORK_FORWARD_NONE = 0, @@ -277,6 +278,8 @@ struct _virNetworkObj { typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; struct _virNetworkObjList { + virObject parent; + size_t count; virNetworkObjPtr *objs; }; @@ -298,19 +301,17 @@ virNetworkObjIsActive(const virNetworkObj *net) return net->active; } -bool virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint); +virNetworkObjListPtr virNetworkObjListNew(void); virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid); virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name); - +bool virNetworkObjTaint(virNetworkObjPtr obj, + virNetworkTaintFlags taint); void virNetworkDefFree(virNetworkDefPtr def); void virNetworkObjFree(virNetworkObjPtr net); -void virNetworkObjListFree(virNetworkObjListPtr nets); - typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ce5e3a..e2b558f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -570,8 +570,8 @@ virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; virNetworkObjListForEach; -virNetworkObjListFree; virNetworkObjListGetNames; +virNetworkObjListNew; virNetworkObjListNumOfNetworks; virNetworkObjListPrune; virNetworkObjLock; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1878833..9637371 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -620,7 +620,7 @@ networkStateInitialize(bool privileged, /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driver->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); - if (VIR_ALLOC(driver->networks) < 0) + if (!(driver->networks = virNetworkObjListNew())) goto error; if (virNetworkLoadAllState(driver->networks, @@ -751,8 +751,7 @@ networkStateCleanup(void) virObjectEventStateFree(driver->networkEventState); /* free inactive networks */ - virNetworkObjListFree(driver->networks); - VIR_FREE(driver->networks); + virObjectUnref(driver->networks); VIR_FREE(driver->networkConfigDir); VIR_FREE(driver->networkAutostartDir); diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 6b53518..62ed268 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -329,7 +329,7 @@ parallelsNetworkOpen(virConnectPtr conn, if (STRNEQ(conn->driver->name, "Parallels")) return VIR_DRV_OPEN_DECLINED; - if (VIR_ALLOC(privconn->networks) < 0) + if (!(privconn->networks = virNetworkObjListNew())) goto error; if (parallelsLoadNetworks(conn->privateData) < 0) @@ -337,7 +337,7 @@ parallelsNetworkOpen(virConnectPtr conn, return VIR_DRV_OPEN_SUCCESS; error: - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); return VIR_DRV_OPEN_DECLINED; } @@ -349,8 +349,7 @@ int parallelsNetworkClose(virConnectPtr conn) return 0; parallelsDriverLock(privconn); - virNetworkObjListFree(privconn->networks); - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); parallelsDriverUnlock(privconn); return 0; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2bfe0ad..693b930 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -725,7 +725,7 @@ testOpenDefault(virConnectPtr conn) goto error; if (!(privconn->domains = virDomainObjListNew()) || - VIR_ALLOC(privconn->networks) < 0) + !(privconn->networks = virNetworkObjListNew())) goto error; memmove(&privconn->nodeInfo, &defaultNodeInfo, sizeof(defaultNodeInfo)); @@ -830,8 +830,7 @@ testOpenDefault(virConnectPtr conn) error: virObjectUnref(privconn->domains); - virNetworkObjListFree(privconn->networks); - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); virNodeDeviceObjListFree(&privconn->devs); @@ -1414,7 +1413,7 @@ testOpenFromFile(virConnectPtr conn, const char *file) conn->privateData = privconn; if (!(privconn->domains = virDomainObjListNew()) || - VIR_ALLOC(privconn->networks) < 0) + !(privconn->networks = virNetworkObjListNew())) goto error; if (!(privconn->caps = testBuildCapabilities(conn))) @@ -1466,8 +1465,7 @@ testOpenFromFile(virConnectPtr conn, const char *file) xmlXPathFreeContext(ctxt); xmlFreeDoc(doc); virObjectUnref(privconn->domains); - virNetworkObjListFree(privconn->networks); - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); VIR_FREE(privconn->path); @@ -1593,8 +1591,7 @@ static int testConnectClose(virConnectPtr conn) virObjectUnref(privconn->xmlopt); virObjectUnref(privconn->domains); virNodeDeviceObjListFree(&privconn->devs); - virNetworkObjListFree(privconn->networks); - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); virObjectEventStateFree(privconn->eventState); -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:09 +0100, Michal Privoznik wrote:
Well, one day this will be self-locking object, but not today. But lets prepare the code for that! Moreover, virNetworkObjListFree() is no longer needed, so turn it into virNetworkObjListDispose().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 1 - src/conf/network_conf.c | 53 +++++++++++++++++++++++++++++---------- src/conf/network_conf.h | 11 ++++---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 5 ++-- src/parallels/parallels_network.c | 7 +++--- src/test/test_driver.c | 13 ++++------ 7 files changed, 57 insertions(+), 35 deletions(-)
I still dislike the fact that you move the virNetworkObjTaint() function but I don't want to waste more time on it than writing this rant. ACK Peter
Now that all the code uses accessors, don't expose the structure anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++++ src/conf/network_conf.h | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f843e3c..9c1d578 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -51,6 +51,13 @@ /* currently, /sbin/tc implementation allows up to 16 bits for minor class size */ #define CLASS_ID_BITMAP_SIZE (1<<16) +struct _virNetworkObjList { + virObject parent; + + size_t count; + virNetworkObjPtr *objs; +}; + VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, "none", "nat", "route", "bridge", "private", "vepa", "passthrough", "hostdev") diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 0c2609a..3575659 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -277,12 +277,6 @@ struct _virNetworkObj { typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; -struct _virNetworkObjList { - virObject parent; - - size_t count; - virNetworkObjPtr *objs; -}; typedef enum { VIR_NETWORK_TAINT_HOOK, /* Hook script was executed over -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:10 +0100, Michal Privoznik wrote:
Now that all the code uses accessors, don't expose the structure anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++++ src/conf/network_conf.h | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-)
ACK, Peter
So far it's just a structure which happens to have 'Obj' in its name, but otherwise it not related to virObject at all. No reference counting, not virObjectLock(), nothing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 - src/conf/network_conf.c | 135 ++++++++++++++++++++------------------ src/conf/network_conf.h | 8 +-- src/libvirt_private.syms | 4 +- src/network/bridge_driver.c | 56 ++++++++-------- src/parallels/parallels_network.c | 16 ++--- src/test/test_driver.c | 32 ++++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 129 insertions(+), 130 deletions(-) diff --git a/cfg.mk b/cfg.mk index 07a794a..6885f9e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -160,7 +160,6 @@ useless_free_options = \ --name=virNWFilterRuleDefFree \ --name=virNWFilterRuleInstFree \ --name=virNetworkDefFree \ - --name=virNetworkObjFree \ --name=virNodeDeviceDefFree \ --name=virNodeDeviceObjFree \ --name=virObjectUnref \ @@ -249,7 +248,6 @@ useless_free_options = \ # y virNetworkDefFree # n virNetworkFree (returns int) # n virNetworkFreeName (returns int) -# y virNetworkObjFree # n virNodeDevCapsDefFree FIXME # y virNodeDeviceDefFree # n virNodeDeviceFree (returns int) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9c1d578..c1dc694 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -80,11 +80,19 @@ VIR_ENUM_IMPL(virNetworkForwardDriverName, VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, "hook-script"); +static virClassPtr virNetworkObjClass; static virClassPtr virNetworkObjListClass; +static void virNetworkObjDispose(void *obj); static void virNetworkObjListDispose(void *obj); static int virNetworkObjOnceInit(void) { + if (!(virNetworkObjClass = virClassNew(virClassForObjectLockable(), + "virNetworkObj", + sizeof(virNetworkObj), + virNetworkObjDispose))) + return -1; + if (!(virNetworkObjListClass = virClassNew(virClassForObject(), "virNetworkObjList", sizeof(virNetworkObjList), @@ -96,6 +104,32 @@ static int virNetworkObjOnceInit(void) VIR_ONCE_GLOBAL_INIT(virNetworkObj) +virNetworkObjPtr +virNetworkObjNew(void) +{ + virNetworkObjPtr net; + + if (virNetworkObjInitialize() < 0) + return NULL; + + if (!(net = virObjectLockableNew(virNetworkObjClass))) + return NULL; + + if (!(net->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) + goto error; + + /* The first three class IDs are already taken */ + ignore_value(virBitmapSetBit(net->class_id, 0)); + ignore_value(virBitmapSetBit(net->class_id, 1)); + ignore_value(virBitmapSetBit(net->class_id, 2)); + + return net; + + error: + virObjectUnref(net); + return NULL; +} + virNetworkObjListPtr virNetworkObjListNew(void) { virNetworkObjListPtr nets; @@ -115,10 +149,10 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, size_t i; for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) return nets->objs[i]; - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } return NULL; @@ -130,10 +164,10 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, size_t i; for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) return nets->objs[i]; - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } return NULL; @@ -297,18 +331,14 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def); } -void virNetworkObjFree(virNetworkObjPtr net) +static void +virNetworkObjDispose(void *obj) { - if (!net) - return; + virNetworkObjPtr net = obj; virNetworkDefFree(net->def); virNetworkDefFree(net->newDef); virBitmapFree(net->class_id); - - virMutexDestroy(&net->lock); - - VIR_FREE(net); } static void @@ -318,7 +348,7 @@ virNetworkObjListDispose(void *obj) size_t i; for (i = 0; i < nets->count; i++) - virNetworkObjFree(nets->objs[i]); + virObjectUnref(nets->objs[i]); VIR_FREE(nets->objs); } @@ -409,32 +439,20 @@ virNetworkAssignDef(virNetworkObjListPtr nets, return network; } - if (VIR_ALLOC(network) < 0) + if (!(network = virNetworkObjNew())) return NULL; - if (virMutexInit(&network->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(network); - return NULL; - } - virNetworkObjLock(network); + virObjectLock(network); - if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0 || - !(network->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) + if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0) goto error; - /* The first three class IDs are already taken */ - ignore_value(virBitmapSetBit(network->class_id, 0)); - ignore_value(virBitmapSetBit(network->class_id, 1)); - ignore_value(virBitmapSetBit(network->class_id, 2)); - network->def = def; network->persistent = !live; return network; error: - virNetworkObjUnlock(network); - virNetworkObjFree(network); + virObjectUnlock(network); + virObjectUnref(network); return NULL; } @@ -602,17 +620,16 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i; - virNetworkObjUnlock(net); + virObjectUnlock(net); for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { - virNetworkObjUnlock(nets->objs[i]); - virNetworkObjFree(nets->objs[i]); - VIR_DELETE_ELEMENT(nets->objs, i, nets->count); + virObjectUnlock(net); + virObjectUnref(net); break; } - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } } @@ -3014,7 +3031,7 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, continue; if ((net = virNetworkLoadState(nets, stateDir, entry->d_name))) - virNetworkObjUnlock(net); + virObjectUnlock(net); } closedir(dir); @@ -3055,7 +3072,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, autostartDir, entry->d_name); if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); } closedir(dir); @@ -3110,12 +3127,12 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, unsigned int ret = 0; for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i]->def->bridge && STREQ(nets->objs[i]->def->bridge, bridge) && !(skipname && STREQ(nets->objs[i]->def->name, skipname))) ret = 1; - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } return ret; @@ -4223,21 +4240,11 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr nets, cleanup: if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } -void virNetworkObjLock(virNetworkObjPtr obj) -{ - virMutexLock(&obj->lock); -} - -void virNetworkObjUnlock(virNetworkObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} - #define MATCH(FLAG) (flags & (FLAG)) static bool virNetworkMatch(virNetworkObjPtr netobj, @@ -4289,21 +4296,21 @@ virNetworkObjListExport(virConnectPtr conn, for (i = 0; i < netobjs->count; i++) { virNetworkObjPtr netobj = netobjs->objs[i]; - virNetworkObjLock(netobj); + virObjectLock(netobj); if ((!filter || filter(conn, netobj->def)) && virNetworkMatch(netobj, flags)) { if (nets) { if (!(net = virGetNetwork(conn, netobj->def->name, netobj->def->uuid))) { - virNetworkObjUnlock(netobj); + virObjectUnlock(netobj); goto cleanup; } tmp_nets[nnets] = net; } nnets++; } - virNetworkObjUnlock(netobj); + virObjectUnlock(netobj); } if (tmp_nets) { @@ -4365,21 +4372,21 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, for (i = 0; i < nets->count && got < nnames; i++) { virNetworkObjPtr obj = nets->objs[i]; - virNetworkObjLock(obj); + virObjectLock(obj); if (filter && !filter(conn, obj->def)) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); continue; } if ((active && virNetworkObjIsActive(obj)) || (!active && !virNetworkObjIsActive(obj))) { if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); goto error; } got++; } - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } return got; @@ -4401,16 +4408,16 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, for (i = 0; i < nets->count; i++) { virNetworkObjPtr obj = nets->objs[i]; - virNetworkObjLock(obj); + virObjectLock(obj); if (filter && !filter(conn, obj->def)) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); continue; } if ((active && virNetworkObjIsActive(obj)) || (!active && !virNetworkObjIsActive(obj))) count++; - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } return count; @@ -4433,15 +4440,15 @@ virNetworkObjListPrune(virNetworkObjListPtr nets, while (i < nets->count) { virNetworkObjPtr obj = nets->objs[i]; - virNetworkObjLock(obj); + virObjectLock(obj); if (virNetworkMatch(obj, flags)) { - virNetworkObjUnlock(obj); - virNetworkObjFree(obj); + virObjectUnlock(obj); + virObjectUnref(obj); VIR_DELETE_ELEMENT(nets->objs, i, nets->count); } else { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); i++; } } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3575659..1423676 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -258,7 +258,7 @@ struct _virNetworkDef { typedef struct _virNetworkObj virNetworkObj; typedef virNetworkObj *virNetworkObjPtr; struct _virNetworkObj { - virMutex lock; + virObjectLockable parent; pid_t dnsmasqPid; pid_t radvdPid; @@ -275,6 +275,8 @@ struct _virNetworkObj { unsigned int taint; }; +virNetworkObjPtr virNetworkObjNew(void); + typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; @@ -305,7 +307,6 @@ bool virNetworkObjTaint(virNetworkObjPtr obj, virNetworkTaintFlags taint); void virNetworkDefFree(virNetworkDefPtr def); -void virNetworkObjFree(virNetworkObjPtr net); typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkDefPtr def); @@ -412,9 +413,6 @@ int virNetworkObjIsDuplicate(virNetworkObjListPtr nets, virNetworkDefPtr def, bool check_active); -void virNetworkObjLock(virNetworkObjPtr obj); -void virNetworkObjUnlock(virNetworkObjPtr obj); - VIR_ENUM_DECL(virNetworkForward) # define VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e2b558f..5dae05d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -565,7 +565,6 @@ virNetworkLoadAllState; virNetworkObjAssignDef; virNetworkObjFindByName; virNetworkObjFindByUUID; -virNetworkObjFree; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; @@ -574,11 +573,10 @@ virNetworkObjListGetNames; virNetworkObjListNew; virNetworkObjListNumOfNetworks; virNetworkObjListPrune; -virNetworkObjLock; +virNetworkObjNew; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; virNetworkObjTaint; -virNetworkObjUnlock; virNetworkObjUnsetDefTransient; virNetworkObjUpdate; virNetworkRemoveInactive; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 9637371..51df46d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -348,9 +348,9 @@ networkUpdateState(virNetworkObjPtr obj, { int ret = -1; - virNetworkObjLock(obj); + virObjectLock(obj); if (!virNetworkObjIsActive(obj)) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return 0; } @@ -403,7 +403,7 @@ networkUpdateState(virNetworkObjPtr obj, ret = 0; cleanup: - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -413,7 +413,7 @@ networkAutostartConfig(virNetworkObjPtr net, { int ret = -1; - virNetworkObjLock(net); + virObjectLock(net); if (net->autostart && !virNetworkObjIsActive(net) && networkStartNetwork(net) < 0) @@ -421,7 +421,7 @@ networkAutostartConfig(virNetworkObjPtr net, ret = 0; cleanup: - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -1745,7 +1745,7 @@ networkRefreshDaemonsHelper(virNetworkObjPtr net, void *opaque ATTRIBUTE_UNUSED) { - virNetworkObjLock(net); + virObjectLock(net); if (virNetworkObjIsActive(net) && ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || @@ -1759,7 +1759,7 @@ networkRefreshDaemonsHelper(virNetworkObjPtr net, networkRefreshDhcpDaemon(net); networkRefreshRadvd(net); } - virNetworkObjUnlock(net); + virObjectUnlock(net); return 0; } @@ -1780,7 +1780,7 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr net, void *opaque ATTRIBUTE_UNUSED) { - virNetworkObjLock(net); + virObjectLock(net); if (virNetworkObjIsActive(net) && ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || @@ -1793,7 +1793,7 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr net, /* failed to add but already logged */ } } - virNetworkObjUnlock(net); + virObjectUnlock(net); return 0; } @@ -2497,7 +2497,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -2523,7 +2523,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -2671,7 +2671,7 @@ static int networkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -2690,7 +2690,7 @@ static int networkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -2960,7 +2960,7 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3017,7 +3017,7 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3078,7 +3078,7 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3250,7 +3250,7 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3285,7 +3285,7 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3336,7 +3336,7 @@ static int networkDestroy(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3365,7 +3365,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -3390,7 +3390,7 @@ static char *networkGetBridgeName(virNetworkPtr net) { cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return bridge; } @@ -3411,7 +3411,7 @@ static int networkGetAutostart(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -3479,7 +3479,7 @@ static int networkSetAutostart(virNetworkPtr net, VIR_FREE(configFile); VIR_FREE(autostartLink); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3652,7 +3652,7 @@ networkGetDHCPLeases(virNetworkPtr network, virJSONValueFree(leases_array); if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return rv; @@ -4125,7 +4125,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; error: @@ -4328,7 +4328,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; error: @@ -4478,7 +4478,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virDomainActualNetDefFree(iface->data.network.actual); iface->data.network.actual = NULL; @@ -4582,7 +4582,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 62ed268..86038bf 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -230,7 +230,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; net->active = 1; net->autostart = 1; - virNetworkObjUnlock(net); + virObjectUnlock(net); return net; cleanup: @@ -265,7 +265,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } net->active = 1; net->autostart = 1; - virNetworkObjUnlock(net); + virObjectUnlock(net); return net; @@ -445,7 +445,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -469,7 +469,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -496,7 +496,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -517,7 +517,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -538,7 +538,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -563,7 +563,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 693b930..20c77de 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -787,7 +787,7 @@ testOpenDefault(virConnectPtr conn) goto error; } netobj->active = 1; - virNetworkObjUnlock(netobj); + virObjectUnlock(netobj); if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML))) goto error; @@ -1155,7 +1155,7 @@ testParseNetworks(testConnPtr privconn, } obj->active = 1; - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } ret = 0; @@ -3508,7 +3508,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, cleanup: if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -3532,7 +3532,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, cleanup: if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -3621,7 +3621,7 @@ static int testNetworkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -3642,7 +3642,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -3675,7 +3675,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); testDriverUnlock(privconn); return ret; } @@ -3708,7 +3708,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); testDriverUnlock(privconn); return ret; } @@ -3746,7 +3746,7 @@ static int testNetworkUndefine(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); testDriverUnlock(privconn); return ret; } @@ -3796,7 +3796,7 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); testDriverUnlock(privconn); return ret; } @@ -3833,7 +3833,7 @@ static int testNetworkCreate(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } @@ -3866,7 +3866,7 @@ static int testNetworkDestroy(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); testDriverUnlock(privconn); return ret; } @@ -3893,7 +3893,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } @@ -3922,7 +3922,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return bridge; } @@ -3947,7 +3947,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } @@ -3972,7 +3972,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 280db30..6df161f 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -40,7 +40,7 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr if (!(dev = virNetworkDefParseString(inXmlData))) goto fail; - if (VIR_ALLOC(obj) < 0) + if (!(obj = virNetworkObjNew())) goto fail; obj->def = dev; @@ -66,7 +66,7 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr VIR_FREE(actual); VIR_FREE(pidfile); virCommandFree(cmd); - virNetworkObjFree(obj); + virObjectUnref(obj); dnsmasqContextFree(dctx); return ret; } diff --git a/tests/objectlocking.ml b/tests/objectlocking.ml index 7826148..fd83d6d 100644 --- a/tests/objectlocking.ml +++ b/tests/objectlocking.ml @@ -84,7 +84,6 @@ let lockedObjMethods = [ *) let objectLockMethods = [ "virDomainObjLock"; - "virNetworkObjLock"; "virStoragePoolObjLock"; "virNodeDevObjLock" ] @@ -99,7 +98,6 @@ let objectLockMethods = [ *) let objectUnlockMethods = [ "virDomainObjUnlock"; - "virNetworkObjUnlock"; "virStoragePoolObjUnlock"; "virNodeDevObjUnlock" ] -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:11 +0100, Michal Privoznik wrote:
So far it's just a structure which happens to have 'Obj' in its name, but otherwise it not related to virObject at all. No reference counting, not virObjectLock(), nothing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 - src/conf/network_conf.c | 135 ++++++++++++++++++++------------------ src/conf/network_conf.h | 8 +-- src/libvirt_private.syms | 4 +- src/network/bridge_driver.c | 56 ++++++++-------- src/parallels/parallels_network.c | 16 ++--- src/test/test_driver.c | 32 ++++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 129 insertions(+), 130 deletions(-)
ACK, Peter
Later we can turn APIs to lock the object if needed instead of relying on caller to mutually exclude itself (probably done by locking a big lock anyway). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c1dc694..88f1689 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -52,7 +52,7 @@ #define CLASS_ID_BITMAP_SIZE (1<<16) struct _virNetworkObjList { - virObject parent; + virObjectLockable parent; size_t count; virNetworkObjPtr *objs; @@ -93,7 +93,7 @@ static int virNetworkObjOnceInit(void) virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObject(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), "virNetworkObjList", sizeof(virNetworkObjList), virNetworkObjListDispose))) @@ -137,7 +137,7 @@ virNetworkObjListPtr virNetworkObjListNew(void) if (virNetworkObjInitialize() < 0) return NULL; - if (!(nets = virObjectNew(virNetworkObjListClass))) + if (!(nets = virObjectLockableNew(virNetworkObjListClass))) return NULL; return nets; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:12 +0100, Michal Privoznik wrote:
Later we can turn APIs to lock the object if needed instead of relying on caller to mutually exclude itself (probably done by locking a big lock anyway).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK if you shuffle this one after the EndAPI addition. Peter
This is practically copy of qemuDomObjEndAPI. The reason why is it so widely available is to avoid code duplication, since the function is going to be called from our bridge driver, test driver and parallels driver too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 20 ++++++++++++++------ src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 88f1689..a821f6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -130,6 +130,16 @@ virNetworkObjNew(void) return NULL; } +void +virNetworkObjEndAPI(virNetworkObjPtr *net) +{ + if (!*net) + return; + + virObjectUnlock(*net); + *net = NULL; +} + virNetworkObjListPtr virNetworkObjListNew(void) { virNetworkObjListPtr nets; @@ -3030,8 +3040,8 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, if (!virFileStripSuffix(entry->d_name, ".xml")) continue; - if ((net = virNetworkLoadState(nets, stateDir, entry->d_name))) - virObjectUnlock(net); + net = virNetworkLoadState(nets, stateDir, entry->d_name); + virNetworkObjEndAPI(&net); } closedir(dir); @@ -3071,8 +3081,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, configDir, autostartDir, entry->d_name); - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); } closedir(dir); @@ -4239,8 +4248,7 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr nets, } cleanup: - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); return ret; } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1423676..e0ed714 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -276,6 +276,7 @@ struct _virNetworkObj { }; virNetworkObjPtr virNetworkObjNew(void); +void virNetworkObjEndAPI(virNetworkObjPtr *net); typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5dae05d..c770177 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -563,6 +563,7 @@ virNetworkIpDefPrefix; virNetworkLoadAllConfigs; virNetworkLoadAllState; virNetworkObjAssignDef; +virNetworkObjEndAPI; virNetworkObjFindByName; virNetworkObjFindByUUID; virNetworkObjGetPersistentDef; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:13 +0100, Michal Privoznik wrote:
This is practically copy of qemuDomObjEndAPI. The reason why is it so widely available is to avoid code duplication, since the function is going to be called from our bridge driver, test driver and parallels driver too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 20 ++++++++++++++------ src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 16 insertions(+), 6 deletions(-)
ACK, Peter
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 57 +++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 38 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 51df46d..8501603 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2496,8 +2496,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -2522,8 +2521,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -2670,8 +2668,7 @@ static int networkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -2689,8 +2686,7 @@ static int networkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -2959,8 +2955,7 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) virNetworkDefFree(def); if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3016,8 +3011,7 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) virObjectEventStateQueue(driver->networkEventState, event); if (freeDef) virNetworkDefFree(def); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3077,8 +3071,7 @@ networkUndefine(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3249,8 +3242,7 @@ networkUpdate(virNetworkPtr net, } ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3284,8 +3276,7 @@ static int networkCreate(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3335,8 +3326,7 @@ static int networkDestroy(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3364,8 +3354,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, ret = virNetworkDefFormat(def, flags); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -3389,8 +3378,7 @@ static char *networkGetBridgeName(virNetworkPtr net) { ignore_value(VIR_STRDUP(bridge, network->def->bridge)); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return bridge; } @@ -3410,8 +3398,7 @@ static int networkGetAutostart(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -3478,8 +3465,7 @@ static int networkSetAutostart(virNetworkPtr net, cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3651,8 +3637,7 @@ networkGetDHCPLeases(virNetworkPtr network, VIR_FREE(custom_lease_file); virJSONValueFree(leases_array); - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return rv; @@ -4124,8 +4109,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; error: @@ -4327,8 +4311,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; error: @@ -4477,8 +4460,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, } ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virDomainActualNetDefFree(iface->data.network.actual); iface->data.network.actual = NULL; @@ -4581,8 +4563,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:14 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 57 +++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 38 deletions(-)
ACK, Peter
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 20c77de..72f40ed 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3507,8 +3507,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, net->def->name, net->def->uuid); cleanup: - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); return ret; } @@ -3531,8 +3530,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, net->def->name, net->def->uuid); cleanup: - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); return ret; } @@ -3620,8 +3618,7 @@ static int testNetworkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -3641,8 +3638,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -3674,8 +3670,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) virNetworkDefFree(def); if (event) testObjectEventQueue(privconn, event); - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); testDriverUnlock(privconn); return ret; } @@ -3707,8 +3702,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) virNetworkDefFree(def); if (event) testObjectEventQueue(privconn, event); - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); testDriverUnlock(privconn); return ret; } @@ -3745,8 +3739,7 @@ static int testNetworkUndefine(virNetworkPtr network) cleanup: if (event) testObjectEventQueue(privconn, event); - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); testDriverUnlock(privconn); return ret; } @@ -3795,8 +3788,7 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); testDriverUnlock(privconn); return ret; } @@ -3832,8 +3824,7 @@ static int testNetworkCreate(virNetworkPtr network) cleanup: if (event) testObjectEventQueue(privconn, event); - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } @@ -3865,8 +3856,7 @@ static int testNetworkDestroy(virNetworkPtr network) cleanup: if (event) testObjectEventQueue(privconn, event); - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); testDriverUnlock(privconn); return ret; } @@ -3892,8 +3882,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, ret = virNetworkDefFormat(privnet->def, flags); cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } @@ -3921,8 +3910,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { ignore_value(VIR_STRDUP(bridge, privnet->def->bridge)); cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return bridge; } @@ -3946,8 +3934,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, ret = 0; cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } @@ -3971,8 +3958,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, ret = 0; cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:15 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-)
ACK, Peter
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 86038bf..1bcd2d3 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -230,7 +230,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; net->active = 1; net->autostart = 1; - virObjectUnlock(net); return net; cleanup: @@ -241,7 +240,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) static virNetworkObjPtr parallelsAddRoutedNetwork(parallelsConnPtr privconn) { - virNetworkObjPtr net; + virNetworkObjPtr net = NULL; virNetworkDefPtr def; if (VIR_ALLOC(def) < 0) @@ -265,7 +264,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } net->active = 1; net->autostart = 1; - virObjectUnlock(net); return net; @@ -277,7 +275,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) static int parallelsLoadNetworks(parallelsConnPtr privconn) { virJSONValuePtr jobj, jobj2; - virNetworkObjPtr net; + virNetworkObjPtr net = NULL; int ret = -1; int count; size_t i; @@ -305,16 +303,19 @@ static int parallelsLoadNetworks(parallelsConnPtr privconn) net = parallelsLoadNetwork(privconn, jobj2); if (!net) goto cleanup; + else + virNetworkObjEndAPI(&net); } - if (!parallelsAddRoutedNetwork(privconn)) + if (!(net = parallelsAddRoutedNetwork(privconn))) goto cleanup; ret = 0; cleanup: virJSONValueFree(jobj); + virNetworkObjEndAPI(&net); return ret; } @@ -444,8 +445,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -468,8 +468,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -495,8 +494,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, ret = virNetworkDefFormat(network->def, flags); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -516,8 +514,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -537,8 +534,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -562,8 +558,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:16 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
For this patch this isn't entirely true, as ...
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 86038bf..1bcd2d3 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -230,7 +230,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; net->active = 1; net->autostart = 1; - virObjectUnlock(net);
... this ...
return net;
cleanup: @@ -241,7 +240,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) static virNetworkObjPtr parallelsAddRoutedNetwork(parallelsConnPtr privconn) { - virNetworkObjPtr net; + virNetworkObjPtr net = NULL; virNetworkDefPtr def;
if (VIR_ALLOC(def) < 0) @@ -265,7 +264,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } net->active = 1; net->autostart = 1; - virObjectUnlock(net);
return net;
@@ -277,7 +275,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) static int parallelsLoadNetworks(parallelsConnPtr privconn) { virJSONValuePtr jobj, jobj2; - virNetworkObjPtr net; + virNetworkObjPtr net = NULL; int ret = -1; int count; size_t i; @@ -305,16 +303,19 @@ static int parallelsLoadNetworks(parallelsConnPtr privconn) net = parallelsLoadNetwork(privconn, jobj2); if (!net) goto cleanup; + else + virNetworkObjEndAPI(&net);
... and this are semantic changes.
}
- if (!parallelsAddRoutedNetwork(privconn)) + if (!(net = parallelsAddRoutedNetwork(privconn))) goto cleanup;
ret = 0;
cleanup: virJSONValueFree(jobj); + virNetworkObjEndAPI(&net); return ret; }
They make sense though. ACK. Peter
This is going to be needed later, when some functions needs to avoid calling multiple times at once. It will work like this: 1) gain the object list mutex 2) find the object to work on 3) do the work 4) release the mutex As an example of such function is virNetworkAssignDef(). The other use case might be in virNetworkObjListForEach() callback. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 45 ++++++++++++++++++++++++++++++++++----------- src/conf/network_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a821f6c..8cf9ffd 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -153,34 +153,57 @@ virNetworkObjListPtr virNetworkObjListNew(void) return nets; } +virNetworkObjPtr +virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, + const unsigned char *uuid) +{ + size_t i; + + for (i = 0; i < nets->count; i++) { + virObjectLock(nets->objs[i]); + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) + return nets->objs[i]; + virObjectUnlock(nets->objs[i]); + } + + return NULL; +} + virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { + virNetworkObjPtr ret; + + virObjectLock(nets); + ret = virNetworkObjFindByUUIDLocked(nets, uuid); + virObjectUnlock(nets); + return ret; +} + +virNetworkObjPtr +virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, + const char *name) +{ size_t i; for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) + if (STREQ(nets->objs[i]->def->name, name)) return nets->objs[i]; virObjectUnlock(nets->objs[i]); } return NULL; } - virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) { - size_t i; + virNetworkObjPtr ret; - for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); - if (STREQ(nets->objs[i]->def->name, name)) - return nets->objs[i]; - virObjectUnlock(nets->objs[i]); - } - - return NULL; + virObjectLock(nets); + ret = virNetworkObjFindByNameLocked(nets, name); + virObjectUnlock(nets); + return ret; } bool diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e0ed714..3e926f7 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -300,8 +300,12 @@ 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); bool virNetworkObjTaint(virNetworkObjPtr obj, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c770177..c678dd8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -565,7 +565,9 @@ virNetworkLoadAllState; virNetworkObjAssignDef; virNetworkObjEndAPI; virNetworkObjFindByName; +virNetworkObjFindByNameLocked; virNetworkObjFindByUUID; +virNetworkObjFindByUUIDLocked; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:17 +0100, Michal Privoznik wrote:
This is going to be needed later, when some functions needs to avoid calling multiple times at once. It will work like this:
1) gain the object list mutex 2) find the object to work on 3) do the work 4) release the mutex
As an example of such function is virNetworkAssignDef(). The other use case might be in virNetworkObjListForEach() callback.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 45 ++++++++++++++++++++++++++++++++++----------- src/conf/network_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a821f6c..8cf9ffd 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -153,34 +153,57 @@ virNetworkObjListPtr virNetworkObjListNew(void) return nets; }
+virNetworkObjPtr +virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, + const unsigned char *uuid) +{ + size_t i; + + for (i = 0; i < nets->count; i++) { + virObjectLock(nets->objs[i]); + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) + return nets->objs[i]; + virObjectUnlock(nets->objs[i]); + } + + return NULL; +} + virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { + virNetworkObjPtr ret; + + virObjectLock(nets); + ret = virNetworkObjFindByUUIDLocked(nets, uuid); + virObjectUnlock(nets); + return ret; +} + +virNetworkObjPtr +virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, + const char *name) +{ size_t i;
for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) + if (STREQ(nets->objs[i]->def->name, name)) return nets->objs[i]; virObjectUnlock(nets->objs[i]); }
return NULL; } -
This deletes the whitespace between functions.
virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name)a
ACK without damaging the serenity of whitespace :D Peter
On Thu, Mar 05, 2015 at 12:05:17 +0100, Michal Privoznik wrote:
This is going to be needed later, when some functions needs to avoid calling multiple times at once. It will work like this:
1) gain the object list mutex 2) find the object to work on 3) do the work 4) release the mutex
As an example of such function is virNetworkAssignDef(). The other use case might be in virNetworkObjListForEach() callback.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 45 ++++++++++++++++++++++++++++++++++----------- src/conf/network_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a821f6c..8cf9ffd 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -153,34 +153,57 @@ virNetworkObjListPtr virNetworkObjListNew(void) return nets; }
+virNetworkObjPtr +virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, + const unsigned char *uuid) +{ + size_t i; + + for (i = 0; i < nets->count; i++) { + virObjectLock(nets->objs[i]); + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) + return nets->objs[i];
This also creates a deadlock that you fix in the next one. Looks like a rebase artifact.
+ virObjectUnlock(nets->objs[i]); + } + + return NULL; +}
On Thu, Mar 05, 2015 at 16:58:08 +0100, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:17 +0100, Michal Privoznik wrote:
This is going to be needed later, when some functions needs to avoid calling multiple times at once. It will work like this:
1) gain the object list mutex 2) find the object to work on 3) do the work 4) release the mutex
As an example of such function is virNetworkAssignDef(). The other use case might be in virNetworkObjListForEach() callback.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 45 ++++++++++++++++++++++++++++++++++----------- src/conf/network_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a821f6c..8cf9ffd 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -153,34 +153,57 @@ virNetworkObjListPtr virNetworkObjListNew(void) return nets; }
+virNetworkObjPtr +virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, + const unsigned char *uuid) +{ + size_t i; + + for (i = 0; i < nets->count; i++) { + virObjectLock(nets->objs[i]); + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) + return nets->objs[i];
This also creates a deadlock that you fix in the next one. Looks like a rebase artifact.
Disregard this please, the locking is right, just the changes in the next patch confused me at first. Peter
Every API that touches internal structure of the object must lock the object first. Not every API that has the object as an argument needs to do that though. Some APIs just pass the object to lower layers which, however, must lock the object then. Look at the code, you'll get my meaning soon. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8cf9ffd..7af303e 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -157,16 +157,19 @@ virNetworkObjPtr virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, const unsigned char *uuid) { + virNetworkObjPtr ret = NULL; size_t i; for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return nets->objs[i]; + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); } - return NULL; + return ret; } virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, @@ -184,16 +187,19 @@ virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, const char *name) { + virNetworkObjPtr ret = NULL; size_t i; for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (STREQ(nets->objs[i]->def->name, name)) - return nets->objs[i]; + if (STREQ(nets->objs[i]->def->name, name)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); } - return NULL; + return ret; } virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) @@ -467,13 +473,17 @@ virNetworkAssignDef(virNetworkObjListPtr nets, { virNetworkObjPtr network; - if ((network = virNetworkObjFindByName(nets, def->name))) { + virObjectLock(nets); + if ((network = virNetworkObjFindByNameLocked(nets, def->name))) { + virObjectUnlock(nets); virNetworkObjAssignDef(network, def, live); return network; } - if (!(network = virNetworkObjNew())) + if (!(network = virNetworkObjNew())) { + virObjectUnlock(nets); return NULL; + } virObjectLock(network); if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0) @@ -481,9 +491,11 @@ virNetworkAssignDef(virNetworkObjListPtr nets, network->def = def; network->persistent = !live; + virObjectUnlock(nets); return network; error: + virObjectUnlock(nets); virObjectUnlock(network); virObjectUnref(network); return NULL; @@ -654,6 +666,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, size_t i; virObjectUnlock(net); + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { @@ -664,6 +677,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, } virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets); } /* return ips[index], or NULL if there aren't enough ips */ @@ -3158,6 +3172,7 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, size_t i; unsigned int ret = 0; + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); if (nets->objs[i]->def->bridge && @@ -3166,6 +3181,7 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, ret = 1; virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets); return ret; } @@ -4325,6 +4341,7 @@ virNetworkObjListExport(virConnectPtr conn, if (nets && VIR_ALLOC_N(tmp_nets, netobjs->count + 1) < 0) goto cleanup; + virObjectLock(netobjs); for (i = 0; i < netobjs->count; i++) { virNetworkObjPtr netobj = netobjs->objs[i]; virObjectLock(netobj); @@ -4335,6 +4352,7 @@ virNetworkObjListExport(virConnectPtr conn, netobj->def->name, netobj->def->uuid))) { virObjectUnlock(netobj); + virObjectUnlock(netobjs); goto cleanup; } tmp_nets[nnets] = net; @@ -4343,6 +4361,7 @@ virNetworkObjListExport(virConnectPtr conn, } virObjectUnlock(netobj); } + virObjectUnlock(netobjs); if (tmp_nets) { /* trim the array to the final size */ @@ -4370,7 +4389,9 @@ virNetworkObjListExport(virConnectPtr conn, * @opaque: pointer to pass to the @callback * * Function iterates over the list of network objects and calls - * passed callback over each one of them. + * passed callback over each one of them. You should avoid + * calling those virNetworkObjList APIs, which lock the list + * again in favor of their virNetworkObj*Locked variants. * * Returns: 0 on success, -1 otherwise. */ @@ -4382,10 +4403,12 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, int ret = 0; size_t i = 0; + virObjectLock(nets); for (i = 0; i < nets->count; i++) { if (callback(nets->objs[i], opaque) < 0) ret = -1; } + virObjectUnlock(nets); return ret; } @@ -4401,6 +4424,7 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, int got = 0; size_t i; + virObjectLock(nets); for (i = 0; i < nets->count && got < nnames; i++) { virNetworkObjPtr obj = nets->objs[i]; virObjectLock(obj); @@ -4413,12 +4437,14 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, (!active && !virNetworkObjIsActive(obj))) { if (VIR_STRDUP(names[got], obj->def->name) < 0) { virObjectUnlock(obj); + virObjectUnlock(nets); goto error; } got++; } virObjectUnlock(obj); } + virObjectUnlock(nets); return got; @@ -4437,6 +4463,7 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, int count = 0; size_t i; + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virNetworkObjPtr obj = nets->objs[i]; virObjectLock(obj); @@ -4450,6 +4477,7 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, count++; virObjectUnlock(obj); } + virObjectUnlock(nets); return count; } -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:18 +0100, Michal Privoznik wrote:
Every API that touches internal structure of the object must lock the object first. Not every API that has the object as an argument needs to do that though. Some APIs just pass the object to lower layers which, however, must lock the object then. Look at the code, you'll get my meaning soon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8cf9ffd..7af303e 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -157,16 +157,19 @@ virNetworkObjPtr virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, const unsigned char *uuid) { + virNetworkObjPtr ret = NULL; size_t i;
for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return nets->objs[i]; + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); }
This hunk ...
- return NULL; + return ret; }
virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, @@ -184,16 +187,19 @@ virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, const char *name) { + virNetworkObjPtr ret = NULL; size_t i;
for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (STREQ(nets->objs[i]->def->name, name)) - return nets->objs[i]; + if (STREQ(nets->objs[i]->def->name, name)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); }
and this hunk have no semantic impact. I presume they are an artifact of the changes from previous version. I'd appreciate if you'd remove them but I won't insist.
- return NULL; + return ret; }
ACK Peter
On Fri, Mar 06, 2015 at 09:42:24 +0100, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:18 +0100, Michal Privoznik wrote:
Every API that touches internal structure of the object must lock the object first. Not every API that has the object as an argument needs to do that though. Some APIs just pass the object to lower layers which, however, must lock the object then. Look at the code, you'll get my meaning soon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8cf9ffd..7af303e 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -157,16 +157,19 @@ virNetworkObjPtr virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, const unsigned char *uuid) { + virNetworkObjPtr ret = NULL; size_t i;
for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return nets->objs[i]; + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); }
This hunk ...
- return NULL; + return ret; }
virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, @@ -184,16 +187,19 @@ virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, const char *name) { + virNetworkObjPtr ret = NULL; size_t i;
for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (STREQ(nets->objs[i]->def->name, name)) - return nets->objs[i]; + if (STREQ(nets->objs[i]->def->name, name)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); }
and this hunk have no semantic impact. I presume they are an artifact of the changes from previous version. I'd appreciate if you'd remove them but I won't insist.
They make sense in the next patch though ... so they are just misplaced. I don't think it's worth moving them though. Peter
This patch turns both virNetworkObjFindByUUID() and virNetworkObjFindByName() to return an referenced object so that even if caller unlocks it, it's for sure that object won't disappear meanwhile. Especially if the object (in general) is locked and unlocked during the caller run. Moreover, this commit is nicely small, since the object unrefing can be done in virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++-- src/network/bridge_driver.c | 18 +++++------------- src/test/test_driver.c | 10 ++++------ 3 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7af303e..3d318ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -137,6 +137,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return; virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; } @@ -164,6 +165,7 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -194,6 +196,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -491,13 +494,13 @@ virNetworkAssignDef(virNetworkObjListPtr nets, network->def = def; network->persistent = !live; + virObjectRef(network); virObjectUnlock(nets); return network; error: virObjectUnlock(nets); - virObjectUnlock(network); - virObjectUnref(network); + virNetworkObjEndAPI(&network); return NULL; } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8501603..d3f3f4a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2939,7 +2939,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkStartNetwork(network) < 0) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; } @@ -2988,7 +2987,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { if (!virNetworkObjIsActive(network)) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; } /* if network was active already, just undo new persistent @@ -3053,11 +3051,8 @@ networkUndefine(virNetworkPtr net) VIR_INFO("Undefining network '%s'", network->def->name); if (!active) { - if (networkRemoveInactive(network) < 0) { - network = NULL; + if (networkRemoveInactive(network) < 0) goto cleanup; - } - network = NULL; } else { /* if the network still exists, it was active, and we need to make @@ -3314,13 +3309,10 @@ static int networkDestroy(virNetworkPtr net) VIR_NETWORK_EVENT_STOPPED, 0); - if (!network->persistent) { - if (networkRemoveInactive(network) < 0) { - network = NULL; - ret = -1; - goto cleanup; - } - network = NULL; + if (!network->persistent && + networkRemoveInactive(network) < 0) { + ret = -1; + goto cleanup; } cleanup: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 72f40ed..90af80a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -787,7 +787,7 @@ testOpenDefault(virConnectPtr conn) goto error; } netobj->active = 1; - virObjectUnlock(netobj); + virNetworkObjEndAPI(&netobj); if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML))) goto error; @@ -1155,7 +1155,7 @@ testParseNetworks(testConnPtr privconn, } obj->active = 1; - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); } ret = 0; @@ -3733,7 +3733,6 @@ static int testNetworkUndefine(virNetworkPtr network) 0); virNetworkRemoveInactive(privconn->networks, privnet); - privnet = NULL; ret = 0; cleanup: @@ -3847,10 +3846,9 @@ static int testNetworkDestroy(virNetworkPtr network) event = virNetworkEventLifecycleNew(privnet->def->name, privnet->def->uuid, VIR_NETWORK_EVENT_STOPPED, 0); - if (!privnet->persistent) { + if (!privnet->persistent) virNetworkRemoveInactive(privconn->networks, privnet); - privnet = NULL; - } + ret = 0; cleanup: -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:19 +0100, Michal Privoznik wrote:
This patch turns both virNetworkObjFindByUUID() and virNetworkObjFindByName() to return an referenced object so that even if caller unlocks it, it's for sure that object won't disappear meanwhile. Especially if the object (in general) is locked and unlocked during the caller run. Moreover, this commit is nicely small, since the object unrefing can be done in virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++-- src/network/bridge_driver.c | 18 +++++------------- src/test/test_driver.c | 10 ++++------ 3 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7af303e..3d318ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -137,6 +137,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return;
virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; }
@@ -164,6 +165,7 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -194,6 +196,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -491,13 +494,13 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
network->def = def; network->persistent = !live; + virObjectRef(network); virObjectUnlock(nets); return network;
error: virObjectUnlock(nets); - virObjectUnlock(network); - virObjectUnref(network); + virNetworkObjEndAPI(&network); return NULL;
} diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8501603..d3f3f4a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2939,7 +2939,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkStartNetwork(network) < 0) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; }
@@ -2988,7 +2987,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { if (!virNetworkObjIsActive(network)) { virNetworkRemoveInactive(driver->networks, network);
As virNetworkRemoveInactive() now doesn't free the object due to the existing reference, but unlocks it ...
- network = NULL;
... and you don't invalidate the pointer, you'd touch the unlocked object in virNetworkObjEndAPI() and try to unlock it once more.
goto cleanup; } /* if network was active already, just undo new persistent
NACK, virNetworkRemoveInactive needs to be fixed first. Peter
On 06.03.2015 10:23, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:19 +0100, Michal Privoznik wrote:
This patch turns both virNetworkObjFindByUUID() and virNetworkObjFindByName() to return an referenced object so that even if caller unlocks it, it's for sure that object won't disappear meanwhile. Especially if the object (in general) is locked and unlocked during the caller run. Moreover, this commit is nicely small, since the object unrefing can be done in virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++-- src/network/bridge_driver.c | 18 +++++------------- src/test/test_driver.c | 10 ++++------ 3 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7af303e..3d318ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -137,6 +137,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return;
virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; }
@@ -164,6 +165,7 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -194,6 +196,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -491,13 +494,13 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
network->def = def; network->persistent = !live; + virObjectRef(network); virObjectUnlock(nets); return network;
error: virObjectUnlock(nets); - virObjectUnlock(network); - virObjectUnref(network); + virNetworkObjEndAPI(&network); return NULL;
} diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8501603..d3f3f4a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2939,7 +2939,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkStartNetwork(network) < 0) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; }
@@ -2988,7 +2987,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { if (!virNetworkObjIsActive(network)) { virNetworkRemoveInactive(driver->networks, network);
As virNetworkRemoveInactive() now doesn't free the object due to the existing reference, but unlocks it ...
- network = NULL;
... and you don't invalidate the pointer, you'd touch the unlocked object in virNetworkObjEndAPI() and try to unlock it once more.
goto cleanup; } /* if network was active already, just undo new persistent
NACK, virNetworkRemoveInactive needs to be fixed first.
Correct. Nice catch! Would I make you reconsider NACK if I squash this into this commit? diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..20a257b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -668,17 +668,18 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i; + /* It's not needed to reference the @net as we are called + * from an API which surely already has at least one + * reference to the object. */ virObjectUnlock(net); virObjectLock(nets); + virObjectLock(net); for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { VIR_DELETE_ELEMENT(nets->objs, i, nets->count); - virObjectUnlock(net); virObjectUnref(net); break; } - virObjectUnlock(nets->objs[i]); } virObjectUnlock(nets); } Michal
On Fri, Mar 06, 2015 at 11:12:21 +0100, Michal Privoznik wrote:
On 06.03.2015 10:23, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:19 +0100, Michal Privoznik wrote:
This patch turns both virNetworkObjFindByUUID() and virNetworkObjFindByName() to return an referenced object so that even if caller unlocks it, it's for sure that object won't disappear meanwhile. Especially if the object (in general) is locked and unlocked during the caller run. Moreover, this commit is nicely small, since the object unrefing can be done in virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++-- src/network/bridge_driver.c | 18 +++++------------- src/test/test_driver.c | 10 ++++------ 3 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7af303e..3d318ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -137,6 +137,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return;
virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; }
@@ -164,6 +165,7 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -194,6 +196,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -491,13 +494,13 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
network->def = def; network->persistent = !live; + virObjectRef(network); virObjectUnlock(nets); return network;
error: virObjectUnlock(nets); - virObjectUnlock(network); - virObjectUnref(network); + virNetworkObjEndAPI(&network); return NULL;
} diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8501603..d3f3f4a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2939,7 +2939,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkStartNetwork(network) < 0) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; }
@@ -2988,7 +2987,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { if (!virNetworkObjIsActive(network)) { virNetworkRemoveInactive(driver->networks, network);
As virNetworkRemoveInactive() now doesn't free the object due to the existing reference, but unlocks it ...
- network = NULL;
... and you don't invalidate the pointer, you'd touch the unlocked object in virNetworkObjEndAPI() and try to unlock it once more.
goto cleanup; } /* if network was active already, just undo new persistent
NACK, virNetworkRemoveInactive needs to be fixed first.
Correct. Nice catch! Would I make you reconsider NACK if I squash this into this commit?
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..20a257b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -668,17 +668,18 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i;
+ /* It's not needed to reference the @net as we are called + * from an API which surely already has at least one + * reference to the object. */ virObjectUnlock(net); virObjectLock(nets); + virObjectLock(net); for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { VIR_DELETE_ELEMENT(nets->objs, i, nets->count); - virObjectUnlock(net); virObjectUnref(net); break; } - virObjectUnlock(nets->objs[i]); } virObjectUnlock(nets); }
Fair enough. ACK with the above fixup. Peter
Now that we have fine grained locks, there's no need to lock the whole driver. We can rely on self-locking APIs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 49 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 42 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d3f3f4a..529ba2b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -129,9 +129,7 @@ networkObjFromNetwork(virNetworkPtr net) virNetworkObjPtr network; char uuidstr[VIR_UUID_STRING_BUFLEN]; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); - networkDriverUnlock(); if (!network) { virUUIDFormat(net->uuid, uuidstr); @@ -264,6 +262,11 @@ networkRemoveInactive(virNetworkObjPtr net) int ret = -1; + virObjectRef(net); + virObjectUnlock(net); + networkDriverLock(); + virObjectLock(net); + /* remove the (possibly) existing dnsmasq and radvd files */ if (!(dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir))) { @@ -315,6 +318,8 @@ networkRemoveInactive(virNetworkObjPtr net) VIR_FREE(radvdpidbase); VIR_FREE(statusfile); dnsmasqContextFree(dctx); + networkDriverUnlock(); + virObjectUnref(net); return ret; } @@ -700,11 +705,9 @@ networkStateAutoStart(void) if (!driver) return; - networkDriverLock(); virNetworkObjListForEach(driver->networks, networkAutostartConfig, NULL); - networkDriverUnlock(); } /** @@ -2478,9 +2481,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, uuid); - networkDriverUnlock(); if (!network) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); @@ -2506,9 +2507,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), name); @@ -2532,12 +2531,10 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) if (virConnectNumOfNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); nactive = virNetworkObjListNumOfNetworks(driver->networks, true, virConnectNumOfNetworksCheckACL, conn); - networkDriverUnlock(); return nactive; } @@ -2548,12 +2545,10 @@ static int networkConnectListNetworks(virConnectPtr conn, char **const names, in if (virConnectListNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); got = virNetworkObjListGetNames(driver->networks, true, names, nnames, virConnectListNetworksCheckACL, conn); - networkDriverUnlock(); return got; } @@ -2565,12 +2560,10 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) if (virConnectNumOfDefinedNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); ninactive = virNetworkObjListNumOfNetworks(driver->networks, false, virConnectNumOfDefinedNetworksCheckACL, conn); - networkDriverUnlock(); return ninactive; } @@ -2581,12 +2574,10 @@ static int networkConnectListDefinedNetworks(virConnectPtr conn, char **const na if (virConnectListDefinedNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); got = virNetworkObjListGetNames(driver->networks, false, names, nnames, virConnectListDefinedNetworksCheckACL, conn); - networkDriverUnlock(); return got; } @@ -2602,11 +2593,9 @@ networkConnectListAllNetworks(virConnectPtr conn, if (virConnectListAllNetworksEnsureACL(conn) < 0) goto cleanup; - networkDriverLock(); ret = virNetworkObjListExport(conn, driver->networks, nets, virConnectListAllNetworksCheckACL, flags); - networkDriverUnlock(); cleanup: return ret; @@ -2917,8 +2906,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup; @@ -2955,7 +2942,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -2967,8 +2953,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup; @@ -3010,7 +2994,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3022,8 +3005,6 @@ networkUndefine(virNetworkPtr net) bool active = false; virObjectEventPtr event = NULL; - networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3067,7 +3048,6 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3091,8 +3071,6 @@ networkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1); - networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3238,7 +3216,6 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3248,7 +3225,6 @@ static int networkCreate(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { @@ -3272,7 +3248,6 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3282,7 +3257,6 @@ static int networkDestroy(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { @@ -3319,7 +3293,6 @@ static int networkDestroy(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3729,9 +3702,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, virDomainActualNetDefFree(iface->data.network.actual); iface->data.network.actual = NULL; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, iface->data.network.name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), @@ -4137,9 +4108,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) return 0; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, iface->data.network.name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), @@ -4336,9 +4305,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) return 0; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, iface->data.network.name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), @@ -4494,9 +4461,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) char *dev_name = NULL; *netaddr = NULL; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, netname); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:20 +0100, Michal Privoznik wrote:
Now that we have fine grained locks, there's no need to lock the whole driver. We can rely on self-locking APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 49 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 42 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d3f3f4a..529ba2b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -129,9 +129,7 @@ networkObjFromNetwork(virNetworkPtr net) virNetworkObjPtr network; char uuidstr[VIR_UUID_STRING_BUFLEN];
- networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); - networkDriverUnlock();
if (!network) { virUUIDFormat(net->uuid, uuidstr); @@ -264,6 +262,11 @@ networkRemoveInactive(virNetworkObjPtr net)
int ret = -1;
+ virObjectRef(net);
Since you've already added proper reference counting in the previous patch and the callers of this function will always have the reference you don't need to ref it here ...
+ virObjectUnlock(net); + networkDriverLock(); + virObjectLock(net);
... also since there are only two functions that call tis piece of code you can avoid removing the network lock from them and avoid doing this locking dance.
+ /* remove the (possibly) existing dnsmasq and radvd files */ if (!(dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir))) {
...
@@ -700,11 +705,9 @@ networkStateAutoStart(void) if (!driver) return;
- networkDriverLock(); virNetworkObjListForEach(driver->networks, networkAutostartConfig,
In the callback above the networkStartNetwork() function is called that accesses the network driver via the reference in the global variable, but the accessed member is driver->stateDir which is immutable when looking at the code. Please annotate (in a separate patch) that the following fields are immutable: driver->networkConfigDir driver->networkAutostartDir driver->stateDir driver->pidDir driver->dnsmasqStateDir driver->radvdStateDir and that the network list is self locking so that people after me won't need to find it out again. ACK Peter
Well, if 'everywhere' is defined as that part of the driver code that serves virNetwork* APIs. Again, we lower layers already have their locks, so there's no point doing big lock. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 56 +------------------------------------------------- 1 file changed, 1 insertion(+), 55 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 90af80a..187bd3d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3495,10 +3495,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, virNetworkObjPtr net; virNetworkPtr ret = NULL; - testDriverLock(privconn); net = virNetworkObjFindByUUID(privconn->networks, uuid); - testDriverUnlock(privconn); - if (net == NULL) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3518,10 +3515,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, virNetworkObjPtr net; virNetworkPtr ret = NULL; - testDriverLock(privconn); net = virNetworkObjFindByName(privconn->networks, name); - testDriverUnlock(privconn); - if (net == NULL) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3540,11 +3534,8 @@ static int testConnectNumOfNetworks(virConnectPtr conn) testConnPtr privconn = conn->privateData; int numActive; - testDriverLock(privconn); numActive = virNetworkObjListNumOfNetworks(privconn->networks, true, NULL, conn); - testDriverUnlock(privconn); - return numActive; } @@ -3552,11 +3543,8 @@ static int testConnectListNetworks(virConnectPtr conn, char **const names, int n testConnPtr privconn = conn->privateData; int n; - testDriverLock(privconn); n = virNetworkObjListGetNames(privconn->networks, true, names, nnames, NULL, conn); - testDriverUnlock(privconn); - return n; } @@ -3565,11 +3553,8 @@ static int testConnectNumOfDefinedNetworks(virConnectPtr conn) testConnPtr privconn = conn->privateData; int numInactive; - testDriverLock(privconn); numInactive = virNetworkObjListNumOfNetworks(privconn->networks, false, NULL, conn); - testDriverUnlock(privconn); - return numInactive; } @@ -3577,11 +3562,8 @@ static int testConnectListDefinedNetworks(virConnectPtr conn, char **const names testConnPtr privconn = conn->privateData; int n; - testDriverLock(privconn); n = virNetworkObjListGetNames(privconn->networks, false, names, nnames, NULL, conn); - testDriverUnlock(privconn); - return n; } @@ -3591,15 +3573,10 @@ testConnectListAllNetworks(virConnectPtr conn, unsigned int flags) { testConnPtr privconn = conn->privateData; - int ret = -1; virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); - testDriverLock(privconn); - ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); - testDriverUnlock(privconn); - - return ret; + return virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); } static int testNetworkIsActive(virNetworkPtr net) @@ -3608,9 +3585,7 @@ static int testNetworkIsActive(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - testDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3628,9 +3603,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - testDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3651,7 +3624,6 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - testDriverLock(privconn); if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup; @@ -3671,7 +3643,6 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&net); - testDriverUnlock(privconn); return ret; } @@ -3684,7 +3655,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - testDriverLock(privconn); if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup; @@ -3703,7 +3673,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&net); - testDriverUnlock(privconn); return ret; } @@ -3714,7 +3683,6 @@ static int testNetworkUndefine(virNetworkPtr network) int ret = -1; virObjectEventPtr event = NULL; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); if (privnet == NULL) { @@ -3739,7 +3707,6 @@ static int testNetworkUndefine(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); - testDriverUnlock(privconn); return ret; } @@ -3759,8 +3726,6 @@ testNetworkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1); - testDriverLock(privconn); - network = virNetworkObjFindByUUID(privconn->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3788,7 +3753,6 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - testDriverUnlock(privconn); return ret; } @@ -3799,10 +3763,7 @@ static int testNetworkCreate(virNetworkPtr network) int ret = -1; virObjectEventPtr event = NULL; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3834,9 +3795,7 @@ static int testNetworkDestroy(virNetworkPtr network) int ret = -1; virObjectEventPtr event = NULL; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3855,7 +3814,6 @@ static int testNetworkDestroy(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); - testDriverUnlock(privconn); return ret; } @@ -3868,10 +3826,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, virCheckFlags(0, NULL); - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3889,10 +3844,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { char *bridge = NULL; virNetworkObjPtr privnet; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3919,10 +3871,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, virNetworkObjPtr privnet; int ret = -1; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3943,10 +3892,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, virNetworkObjPtr privnet; int ret = -1; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:21 +0100, Michal Privoznik wrote:
Well, if 'everywhere' is defined as that part of the driver code that serves virNetwork* APIs. Again, we lower layers already have their locks, so there's no point doing big lock.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 56 +------------------------------------------------- 1 file changed, 1 insertion(+), 55 deletions(-)
ACK, Peter
While in previous commits there were some places that relied on the big lock, in this file there's no such place and the big driver lock can be dropped completely. Yay! Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 1bcd2d3..1d3b694 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -349,9 +349,7 @@ int parallelsNetworkClose(virConnectPtr conn) if (!privconn) return 0; - parallelsDriverLock(privconn); virObjectUnref(privconn->networks); - parallelsDriverUnlock(privconn); return 0; } @@ -360,11 +358,8 @@ static int parallelsConnectNumOfNetworks(virConnectPtr conn) int nactive; parallelsConnPtr privconn = conn->privateData; - parallelsDriverLock(privconn); nactive = virNetworkObjListNumOfNetworks(privconn->networks, true, NULL, conn); - parallelsDriverUnlock(privconn); - return nactive; } @@ -375,11 +370,8 @@ static int parallelsConnectListNetworks(virConnectPtr conn, parallelsConnPtr privconn = conn->privateData; int got; - parallelsDriverLock(privconn); got = virNetworkObjListGetNames(privconn->networks, true, names, nnames, NULL, conn); - parallelsDriverUnlock(privconn); - return got; } @@ -388,11 +380,8 @@ static int parallelsConnectNumOfDefinedNetworks(virConnectPtr conn) int ninactive; parallelsConnPtr privconn = conn->privateData; - parallelsDriverLock(privconn); ninactive = virNetworkObjListNumOfNetworks(privconn->networks, false, NULL, conn); - parallelsDriverUnlock(privconn); - return ninactive; } @@ -403,10 +392,8 @@ static int parallelsConnectListDefinedNetworks(virConnectPtr conn, parallelsConnPtr privconn = conn->privateData; int got; - parallelsDriverLock(privconn); got = virNetworkObjListGetNames(privconn->networks, false, names, nnames, NULL, conn); - parallelsDriverUnlock(privconn); return got; } @@ -415,15 +402,10 @@ static int parallelsConnectListAllNetworks(virConnectPtr conn, unsigned int flags) { parallelsConnPtr privconn = conn->privateData; - int ret = -1; virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); - parallelsDriverLock(privconn); - ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); - parallelsDriverUnlock(privconn); - - return ret; + return virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); } static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, @@ -433,9 +415,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - parallelsDriverLock(privconn); network = virNetworkObjFindByUUID(privconn->networks, uuid); - parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -456,9 +436,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - parallelsDriverLock(privconn); network = virNetworkObjFindByName(privconn->networks, name); - parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), name); @@ -481,10 +459,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, virCheckFlags(VIR_NETWORK_XML_INACTIVE, NULL); - parallelsDriverLock(privconn); network = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); - if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -504,9 +479,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - parallelsDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -524,9 +497,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - parallelsDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -545,9 +516,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, virNetworkObjPtr network; int ret = -1; - parallelsDriverLock(privconn); network = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:22 +0100, Michal Privoznik wrote:
While in previous commits there were some places that relied on the big lock, in this file there's no such place and the big driver lock can be dropped completely. Yay!
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-)
ACK, Peter
Now that the network driver lock is ash heap of history, we can use more of networkObjFromNetwork(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 529ba2b..2eb225f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -130,7 +130,6 @@ networkObjFromNetwork(virNetworkPtr net) char uuidstr[VIR_UUID_STRING_BUFLEN]; network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { virUUIDFormat(net->uuid, uuidstr); virReportError(VIR_ERR_NO_NETWORK, @@ -3005,12 +3004,8 @@ networkUndefine(virNetworkPtr net) bool active = false; virObjectEventPtr event = NULL; - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkUndefineEnsureACL(net->conn, network->def) < 0) goto cleanup; @@ -3071,12 +3066,8 @@ networkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkUpdateEnsureACL(net->conn, network->def, flags) < 0) goto cleanup; @@ -3225,13 +3216,8 @@ static int networkCreate(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkCreateEnsureACL(net->conn, network->def) < 0) goto cleanup; @@ -3257,13 +3243,8 @@ static int networkDestroy(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkDestroyEnsureACL(net->conn, network->def) < 0) goto cleanup; @@ -3375,13 +3356,9 @@ static int networkSetAutostart(virNetworkPtr net, int ret = -1; networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkSetAutostartEnsureACL(net->conn, network->def) < 0) goto cleanup; -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:23 +0100, Michal Privoznik wrote:
Now that the network driver lock is ash heap of history, we can use more of networkObjFromNetwork().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-)
ACK, Peter
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void -virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network); virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3e926f7..c2e1885 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -323,7 +323,8 @@ void virNetworkObjAssignDef(virNetworkObjPtr network, virNetworkDefPtr def, bool live); int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live); -void virNetworkObjUnsetDefTransient(virNetworkObjPtr network); +void virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network); virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network); int virNetworkObjReplacePersistentDef(virNetworkObjPtr network, virNetworkDefPtr def); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2eb225f..c112d50 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2415,7 +2415,7 @@ networkStartNetwork(virNetworkObjPtr network) cleanup: if (ret < 0) { - virNetworkObjUnsetDefTransient(network); + virNetworkObjUnsetDefTransient(driver->networks, network); virErrorPtr save_err = virSaveLastError(); int save_errno = errno; networkShutdownNetwork(network); @@ -2469,7 +2469,7 @@ static int networkShutdownNetwork(virNetworkObjPtr network) VIR_HOOK_SUBOP_END); network->active = 0; - virNetworkObjUnsetDefTransient(network); + virNetworkObjUnsetDefTransient(driver->networks, network); return ret; } -- 2.0.5
On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote:
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void
I've looked through the next patch and you are basically trying to make the name and UUID pointers for domain immutable or at leas write locked ...
-virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network);
But I don't really like pulling in the complexity into this helper.
virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } }
While I like the idea, I'd rather see a conversion to R/W locks or making of the name and UUID pointers immutable than this hack. Peter
On 06.03.2015 14:31, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote:
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void
I've looked through the next patch and you are basically trying to make the name and UUID pointers for domain immutable or at leas write locked ...
-virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network);
But I don't really like pulling in the complexity into this helper.
virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } }
While I like the idea, I'd rather see a conversion to R/W locks or making of the name and UUID pointers immutable than this hack.
Well: 1) We don't have an virObjectRWLockable or something similar. I can add it, but that would postpone merging this patchset for yet another version. 2) Nor UUID nor name can be made immutable, as we are storing just a pointers to network objects in the array. Not UUID or name. It's not a hash table like in virDomainObjList* [1]. And when looking up an object, we access each object's definition directly. Therefore all other places changing definition must lock the object list. I agree, that one day we can change this to RW locks, but then again - that'd require more rework which can be saved for a follow up series. Moreover, if I introduce new RWLockable object, other drivers might benefit from it too. Michal 1: Yes, one day we can turn the array into hash table too. There's plenty of work to be done. I agree. But I prefer it to be divided into smaller pieces instead of this one big patchset of hundreds of patches :-P
On Fri, Mar 06, 2015 at 14:42:34 +0100, Michal Privoznik wrote:
On 06.03.2015 14:31, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote:
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void
I've looked through the next patch and you are basically trying to make the name and UUID pointers for domain immutable or at leas write locked ...
-virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network);
But I don't really like pulling in the complexity into this helper.
virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } }
While I like the idea, I'd rather see a conversion to R/W locks or making of the name and UUID pointers immutable than this hack.
Well:
1) We don't have an virObjectRWLockable or something similar. I can add it, but that would postpone merging this patchset for yet another version.
2) Nor UUID nor name can be made immutable, as we are storing just a pointers to network objects in the array. Not UUID or name. It's not a hash table like in virDomainObjList* [1]. And when looking up an object, we access each object's definition directly. Therefore all other places changing definition must lock the object list.
Since we don't support changing the name nor UUID of a network object (or any other libvirt externaly visible object), you could split them out from being part of the definition and use them in the object itself where you could mark them as immutable.
I agree, that one day we can change this to RW locks, but then again - that'd require more rework which can be saved for a follow up series. Moreover, if I introduce new RWLockable object, other drivers might benefit from it too.
They definitely might benefit from them, but refactoring to a rwlock will be pain as you need to make sure that you mark the parts correctly. Also saving some work to avoid future refactors creates technical debt. I'll rather sacrifice a bit of performance in this case than to introduce a hack.
Michal
Peter
On 06.03.2015 14:47, Peter Krempa wrote:
On Fri, Mar 06, 2015 at 14:42:34 +0100, Michal Privoznik wrote:
On 06.03.2015 14:31, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote:
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void
I've looked through the next patch and you are basically trying to make the name and UUID pointers for domain immutable or at leas write locked ...
-virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network);
But I don't really like pulling in the complexity into this helper.
virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } }
While I like the idea, I'd rather see a conversion to R/W locks or making of the name and UUID pointers immutable than this hack.
Well:
1) We don't have an virObjectRWLockable or something similar. I can add it, but that would postpone merging this patchset for yet another version.
2) Nor UUID nor name can be made immutable, as we are storing just a pointers to network objects in the array. Not UUID or name. It's not a hash table like in virDomainObjList* [1]. And when looking up an object, we access each object's definition directly. Therefore all other places changing definition must lock the object list.
Since we don't support changing the name nor UUID of a network object (or any other libvirt externaly visible object), you could split them out from being part of the definition and use them in the object itself where you could mark them as immutable.
This would be even uglier hack than what I'm introducing IMO.
I agree, that one day we can change this to RW locks, but then again - that'd require more rework which can be saved for a follow up series. Moreover, if I introduce new RWLockable object, other drivers might benefit from it too.
They definitely might benefit from them, but refactoring to a rwlock will be pain as you need to make sure that you mark the parts correctly.
Also saving some work to avoid future refactors creates technical debt. I'll rather sacrifice a bit of performance in this case than to introduce a hack.
Well, it's not a hack rather than something we need to do. Even our domain code is wrong in this respect. I mean: virDomainObjListFindByName() accesses domain's definitions directly. We are just lucky, that this function and some portion of qemuProcessStop() hasn't been scheduled to run at the same time. That portion which switches @def and @newDef pointers in the @vm object. We ought to lock the domain object list there as well. Michal
On Fri, Mar 06, 2015 at 02:42:34PM +0100, Michal Privoznik wrote:
On 06.03.2015 14:31, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote:
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void
I've looked through the next patch and you are basically trying to make the name and UUID pointers for domain immutable or at leas write locked ...
-virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network);
But I don't really like pulling in the complexity into this helper.
virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } }
While I like the idea, I'd rather see a conversion to R/W locks or making of the name and UUID pointers immutable than this hack.
Well:
1) We don't have an virObjectRWLockable or something similar. I can add it, but that would postpone merging this patchset for yet another version.
2) Nor UUID nor name can be made immutable, as we are storing just a pointers to network objects in the array. Not UUID or name. It's not a hash table like in virDomainObjList* [1]. And when looking up an object, we access each object's definition directly. Therefore all other places changing definition must lock the object list.
This is why I changed the virDomainObjList to use a hash instead of a list when I introduced lockless access for domain objects. commit 37abd471656957c76eac687ce2ef94d79c8e2731 Author: Daniel P. Berrange <berrange@redhat.com> Date: Fri Jan 11 13:54:15 2013 +0000 Turn virDomainObjList into an opaque virObject As a step towards making virDomainObjList thread-safe turn it into an opaque virObject, preventing any direct access to its internals. As part of this a new method virDomainObjListForEach is introduced to replace all existing usage of virHashForEach
1: Yes, one day we can turn the array into hash table too. There's plenty of work to be done. I agree. But I prefer it to be divided into smaller pieces instead of this one big patchset of hundreds of patches :-P
I'd rather expect to see virNetworkObjList turned into an opaque struct using a virHashTable internally as the very first patch in the series. Keeping a list which requires linear scans is incompatible with doing fast lockless code IMHO Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06.03.2015 15:15, Daniel P. Berrange wrote:
On Fri, Mar 06, 2015 at 02:42:34PM +0100, Michal Privoznik wrote:
On 06.03.2015 14:31, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote:
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void
I've looked through the next patch and you are basically trying to make the name and UUID pointers for domain immutable or at leas write locked ...
-virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network);
But I don't really like pulling in the complexity into this helper.
virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } }
While I like the idea, I'd rather see a conversion to R/W locks or making of the name and UUID pointers immutable than this hack.
Well:
1) We don't have an virObjectRWLockable or something similar. I can add it, but that would postpone merging this patchset for yet another version.
2) Nor UUID nor name can be made immutable, as we are storing just a pointers to network objects in the array. Not UUID or name. It's not a hash table like in virDomainObjList* [1]. And when looking up an object, we access each object's definition directly. Therefore all other places changing definition must lock the object list.
This is why I changed the virDomainObjList to use a hash instead of a list when I introduced lockless access for domain objects.
commit 37abd471656957c76eac687ce2ef94d79c8e2731 Author: Daniel P. Berrange <berrange@redhat.com> Date: Fri Jan 11 13:54:15 2013 +0000
Turn virDomainObjList into an opaque virObject
As a step towards making virDomainObjList thread-safe turn it into an opaque virObject, preventing any direct access to its internals.
As part of this a new method virDomainObjListForEach is introduced to replace all existing usage of virHashForEach
1: Yes, one day we can turn the array into hash table too. There's plenty of work to be done. I agree. But I prefer it to be divided into smaller pieces instead of this one big patchset of hundreds of patches :-P
I'd rather expect to see virNetworkObjList turned into an opaque struct using a virHashTable internally as the very first patch in the series. Keeping a list which requires linear scans is incompatible with doing fast lockless code IMHO
Yes, this could work. Although, I'm inclined to push patches from beginning till 09/24 and introduce patch turning the array into a hash table right after that. My rationale is that at point of 09/24 whole code uses accessors to the network object list so turning array into hash table could end up being small patch. Objections? Michal
On Mon, Mar 09, 2015 at 09:24:32AM +0100, Michal Privoznik wrote:
On 06.03.2015 15:15, Daniel P. Berrange wrote:
On Fri, Mar 06, 2015 at 02:42:34PM +0100, Michal Privoznik wrote:
On 06.03.2015 14:31, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote:
This patch alone does not make much sense, I know. But it prepares ground for next patch which when looking up a network in the object list will not lock each network separately when accessing its definition. Therefore we must have all the places changing network definition lock the list.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..007cebb 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) * This *undoes* what virNetworkObjSetDefTransient did. */ void
I've looked through the next patch and you are basically trying to make the name and UUID pointers for domain immutable or at leas write locked ...
-virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, + virNetworkObjPtr network) { if (network->newDef) { + virObjectRef(network); + virObjectUnlock(network); + virObjectLock(nets); + virObjectLock(network); + virObjectUnref(network);
But I don't really like pulling in the complexity into this helper.
virNetworkDefFree(network->def); network->def = network->newDef; network->newDef = NULL; + virObjectUnlock(nets); } }
While I like the idea, I'd rather see a conversion to R/W locks or making of the name and UUID pointers immutable than this hack.
Well:
1) We don't have an virObjectRWLockable or something similar. I can add it, but that would postpone merging this patchset for yet another version.
2) Nor UUID nor name can be made immutable, as we are storing just a pointers to network objects in the array. Not UUID or name. It's not a hash table like in virDomainObjList* [1]. And when looking up an object, we access each object's definition directly. Therefore all other places changing definition must lock the object list.
This is why I changed the virDomainObjList to use a hash instead of a list when I introduced lockless access for domain objects.
commit 37abd471656957c76eac687ce2ef94d79c8e2731 Author: Daniel P. Berrange <berrange@redhat.com> Date: Fri Jan 11 13:54:15 2013 +0000
Turn virDomainObjList into an opaque virObject
As a step towards making virDomainObjList thread-safe turn it into an opaque virObject, preventing any direct access to its internals.
As part of this a new method virDomainObjListForEach is introduced to replace all existing usage of virHashForEach
1: Yes, one day we can turn the array into hash table too. There's plenty of work to be done. I agree. But I prefer it to be divided into smaller pieces instead of this one big patchset of hundreds of patches :-P
I'd rather expect to see virNetworkObjList turned into an opaque struct using a virHashTable internally as the very first patch in the series. Keeping a list which requires linear scans is incompatible with doing fast lockless code IMHO
Yes, this could work. Although, I'm inclined to push patches from beginning till 09/24 and introduce patch turning the array into a hash table right after that. My rationale is that at point of 09/24 whole code uses accessors to the network object list so turning array into hash table could end up being small patch. Objections?
That sounds like a reasonable approach - iirc that's the way I did it for the domain object list too. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Ta-Da! Finally the commit you've been waiting for. This is yet one of the bottlenecks. Each API has to go through the list of network objects to find the correct one to work on. But currently it's done in suboptimal way: every single network object is locked, and then compared. If found, it's returned, if not it's unlocked and the loop continues with its sibling. This is not optimal as locking every network object can make us waiting for other APIs finish their job. Therefore we serialize here effectively. Fortunately, with previous patches we are sure that network definition will not change as we traverse the list. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 26 ++++++++++++++++++++------ src/conf/network_conf.h | 1 + 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 007cebb..a1bdaf5 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -162,13 +162,20 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, size_t i; for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; virObjectRef(ret); break; } - virObjectUnlock(nets->objs[i]); + } + + if (ret) { + virObjectLock(ret); + if (ret->removing) { + virObjectUnref(ret); + virObjectUnlock(ret); + ret = NULL; + } } return ret; @@ -193,13 +200,20 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, size_t i; for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; virObjectRef(ret); break; } - virObjectUnlock(nets->objs[i]); + } + + if (ret) { + virObjectLock(ret); + if (ret->removing) { + virObjectUnref(ret); + virObjectUnlock(ret); + ret = NULL; + } } return ret; @@ -675,17 +689,17 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i; + net->removing = true; virObjectUnlock(net); virObjectLock(nets); + virObjectLock(net); for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { VIR_DELETE_ELEMENT(nets->objs, i, nets->count); virObjectUnlock(net); virObjectUnref(net); break; } - virObjectUnlock(nets->objs[i]); } virObjectUnlock(nets); } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c2e1885..29d13c9 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -273,6 +273,7 @@ struct _virNetworkObj { unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */ unsigned int taint; + bool removing; }; virNetworkObjPtr virNetworkObjNew(void); -- 2.0.5
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa