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(a)redhat.com&gt; --- 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

An accessor following pattern laid out by virDomainObjList* APIs. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

An accessor following pattern laid out by virDomainObjList* APIs. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

The API will iterate over the list of network object and remove desired ones from it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

Now that all the code uses accessors, don't expose the structure anyway. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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 also creates a deadlock that you fix in the next one. Looks like a rebase artifact.

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(a)redhat.com&gt; --- 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 Fri, Mar 06, 2015 at 09:42:24 +0100, Peter Krempa wrote: They make sense in the next patch though ... so they are just misplaced. I don't think it's worth moving them though. Peter

On Thu, Mar 05, 2015 at 12:05:19 +0100, Michal Privoznik wrote: As virNetworkRemoveInactive() now doesn't free the object due to the existing reference, but unlocks it ... ... and you don't invalidate the pointer, you'd touch the unlocked object in virNetworkObjEndAPI() and try to unlock it once more. NACK, virNetworkRemoveInactive needs to be fixed first. Peter

On Fri, Mar 06, 2015 at 11:12:21 +0100, Michal Privoznik wrote: Fair enough. ACK with the above fixup. Peter

On Thu, Mar 05, 2015 at 12:05:20 +0100, Michal Privoznik wrote: 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 ... ... 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. ... 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

On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote: 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 ... But I don't really like pulling in the complexity into this helper. 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 Fri, Mar 06, 2015 at 14:42:34 +0100, Michal Privoznik wrote: 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. 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. Peter

On Fri, Mar 06, 2015 at 02:42:34PM +0100, Michal Privoznik wrote: 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(a)redhat.com&gt; 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 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 Mon, Mar 09, 2015 at 09:24:32AM +0100, Michal Privoznik wrote: 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 :|