8:06 a.m.
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The past 24 hours have seen a flurry of libvirtd crash reports from
Fedora users.
https://bugzilla.redhat.com/show_bug.cgi?id=1014933
In one thread we have the libvirtd daemon startup code running, and
it is in the middle of QEMU state initialization.
#9 0xb00882e4 in qemuStateInitialize (privileged=true, callback=0xb77a0420
<daemonInhibitCallback>, opaque=0xb8b1fc98) at qemu/qemu_driver.c:595
driverConf = 0xaf5afcd8 "/etc/libvirt/qemu.conf"
conn = 0x0
ebuf =
"\000\260\025\267\024\071P\257\214\000\000\000\360\316\341\257\335\242\023\267\214\000\000\000\210\177X\257\001\000\000\000l\000\000\000\360\316\341\257\000\260\025\267\264\316\341\257\210\177X\257$\316\341\257$\316\341\257l\000\000\000\304\316\341\257\201\321LRl\000\000\000\235R\022\267\000)\233\351\260\316\341\257\000\000\000\000\253G\022\267\000\260\025\267\340\316\341\257\a\000\000\000\v\260\023\267\000\260\025\267\001\000\000\000\254\325\334\266\000\260\025\267\214\261\023\267
:P\257\037:P\257\000\000\000\000/\261\023\267\000\260\025\267uc\334\266\000\260\025\267A\262\023\267\037:P\257\000\000\000\000\001\000\000\000\000\000\000\000\340\316\341\257\334\316\341\257\001\000\000\000\001\000\000\000\033c\024\267"...
membase = 0x0
mempath = 0x0
cfg = 0xaf509050
run_uid = 4294967295
run_gid = 4294967295
__func__ = "qemuStateInitialize"
__FUNCTION__ = "qemuStateInitialize"
#10 0xb74c5325 in virStateInitialize (privileged=true, callback=callback@entry=0xb77a0420
<daemonInhibitCallback>, opaque=opaque@entry=0xb8b1fc98) at libvirt.c:833
i = 6
__func__ = "virStateInitialize"
#11 0xb77a049e in daemonRunStateInit (opaque=opaque@entry=0xb8b1fc98) at libvirtd.c:876
srv = 0xb8b1fc98
__func__ = "daemonRunStateInit"
In another thread, we have a dbus event being handled by the nwfilter
driver, and the nwfilter driver calls into the QEMU driver....which
has not finished initializing itself yet!
Thread 1 (Thread 0xb6366ac0 (LWP 7041)):
#0 0xb0052861 in virQEMUCloseCallbacksGetForConn (closeCallbacks=0x0, conn=0xb8b2cc20) at
qemu/qemu_conf.c:861
list = 0xb8ac57e8
data = {conn = 0xb8b2cc20, list = 0xb8ac57e8, oom = false}
#1 virQEMUCloseCallbacksRun (closeCallbacks=0x0, conn=conn@entry=0xb8b2cc20,
driver=0xaf50b350) at qemu/qemu_conf.c:890
list = 0xb8b2cc20
i = <optimized out>
__func__ = "virQEMUCloseCallbacksRun"
#2 0xb009df3b in qemuConnectClose (conn=0xb8b2cc20) at qemu/qemu_driver.c:1057
driver = <optimized out>
#3 0xb74babc1 in virConnectDispose (obj=0xb8b2cc20) at datatypes.c:159
conn = 0xb8b2cc20
#4 0xb742f22c in virObjectUnref (anyobj=anyobj@entry=0xb8b2cc20) at util/virobject.c:264
klass = 0xb8b2cba0
obj = 0xb8b2cc20
lastRef = true
__func__ = "virObjectUnref"
#5 0xb74c5811 in virConnectClose (conn=conn@entry=0xb8b2cc20) at libvirt.c:1503
__func__ = "virConnectClose"
__FUNCTION__ = "virConnectClose"
#6 0xb023424e in nwfilterStateReload () at nwfilter/nwfilter_driver.c:301
conn = 0xb8b2cc20
#7 0xb02342fc in nwfilterFirewalldDBusFilter (connection=0xaf501038, message=0xaf503910,
user_data=0x0) at nwfilter/nwfilter_driver.c:90
__func__ = "nwfilterFirewalldDBusFilter"
#8 0xb711efb9 in dbus_connection_dispatch (connection=0xaf501038) at
dbus-connection.c:4631
filter = <optimized out>
next = 0x0
message = 0xaf503910
link = <optimized out>
filter_list_copy = 0xaf5009dc
message_link = 0xaf500a18
result = DBUS_HANDLER_RESULT_NOT_YET_HANDLED
pending = <optimized out>
reply_serial = <optimized out>
status = <optimized out>
found_object = 3071507249
__FUNCTION__ = "dbus_connection_dispatch"
#9 0xb740caeb in virDBusWatchCallback (fdatch=fdatch@entry=8, fd=15, events=1,
opaque=0xaf500ca8) at util/virdbus.c:144
watch = 0xaf500ca8
info = 0xaf500de0
dbus_flags = 1
This DBus event is triggered when the firewalld driver is
reloaded, or restarted.
I confirmed this analysis by adding a sleep(10) to the QEMU
driver startup code, and then triggering a firewalld restart.
Sure enough it crashed & burned with the same trace.
The reason why it has suddenly hit us is that we are unlucky
enough to have a firewalld update in Fedora repos at the same
time as a libvirt update, and lots of people are pulling both
updates down in one yum transaction!
After wasting time figuring out how to avoid the race condition
with mutexes and other synchronization ideas, I realized that
the nwfilter code was in fact bogus.
The only reason it gets a virConnectPtr is so that the code
for reloading filters can access its nwfilterPrivateData
field to get the virNWFilterDriverStatePtr object instance.
This is insanely convoluted, since the nwfilter driver can
trivially pass the driver state instance into the
virNWFilterConfLayerInit method at startup.
Thus these patches just rip out all use of virConnectPtr
from the nwfilter driver code, thus avoiding the race with
the QEMU driver initialization code.
This also fixes the nwfilter driver in cases where the QEMU
driver is disabled, but LXC driver still wants to use nwfilter.
Daniel P. Berrange (3):
Remove virConnectPtr arg from virNWFilterDefParse*
Don't pass virConnectPtr in nwfilter 'struct domUpdateCBStruct'
Remove use of virConnectPtr from all remaining nwfilter code
src/conf/nwfilter_conf.c | 78 ++++++++++++++++------------------
src/conf/nwfilter_conf.h | 24 ++++-------
src/lxc/lxc_driver.c | 3 +-
src/nwfilter/nwfilter_dhcpsnoop.c | 12 +++---
src/nwfilter/nwfilter_driver.c | 49 +++++++++------------
src/nwfilter/nwfilter_gentech_driver.c | 32 +++++++-------
src/nwfilter/nwfilter_gentech_driver.h | 10 ++---
src/nwfilter/nwfilter_learnipaddr.c | 6 +--
src/qemu/qemu_driver.c | 6 ++-
src/uml/uml_driver.c | 3 +-
tests/nwfilterxml2xmltest.c | 2 +-
11 files changed, 102 insertions(+), 123 deletions(-)
--
1.8.3.1
8:06 a.m.
New subject: [libvirt] [PATCH 1/3] Remove virConnectPtr arg from virNWFilterDefParse*
From: "Daniel P. Berrange" <berrange(a)redhat.com>
None of the virNWFilterDefParse* methods require a virConnectPtr
arg, so just drop it
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 15 ++++++---------
src/conf/nwfilter_conf.h | 6 ++----
src/nwfilter/nwfilter_driver.c | 2 +-
src/qemu/qemu_driver.c | 3 +++
tests/nwfilterxml2xmltest.c | 2 +-
5 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 3456b77..c009921 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2634,8 +2634,7 @@ cleanup:
static virNWFilterDefPtr
-virNWFilterDefParse(virConnectPtr conn ATTRIBUTE_UNUSED,
- const char *xmlStr,
+virNWFilterDefParse(const char *xmlStr,
const char *filename) {
virNWFilterDefPtr def = NULL;
xmlDocPtr xml;
@@ -2650,18 +2649,16 @@ virNWFilterDefParse(virConnectPtr conn ATTRIBUTE_UNUSED,
virNWFilterDefPtr
-virNWFilterDefParseString(virConnectPtr conn,
- const char *xmlStr)
+virNWFilterDefParseString(const char *xmlStr)
{
- return virNWFilterDefParse(conn, xmlStr, NULL);
+ return virNWFilterDefParse(xmlStr, NULL);
}
virNWFilterDefPtr
-virNWFilterDefParseFile(virConnectPtr conn,
- const char *filename)
+virNWFilterDefParseFile(const char *filename)
{
- return virNWFilterDefParse(conn, NULL, filename);
+ return virNWFilterDefParse(NULL, filename);
}
@@ -3056,7 +3053,7 @@ virNWFilterObjLoad(virConnectPtr conn,
virNWFilterDefPtr def;
virNWFilterObjPtr nwfilter;
- if (!(def = virNWFilterDefParseFile(conn, path))) {
+ if (!(def = virNWFilterDefParseFile(path))) {
return NULL;
}
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index 5d04cff..faa7527 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -713,10 +713,8 @@ int virNWFilterLoadAllConfigs(virConnectPtr conn,
char *virNWFilterConfigFile(const char *dir,
const char *name);
-virNWFilterDefPtr virNWFilterDefParseString(virConnectPtr conn,
- const char *xml);
-virNWFilterDefPtr virNWFilterDefParseFile(virConnectPtr conn,
- const char *filename);
+virNWFilterDefPtr virNWFilterDefParseString(const char *xml);
+virNWFilterDefPtr virNWFilterDefParseFile(const char *filename);
void virNWFilterObjLock(virNWFilterObjPtr obj);
void virNWFilterObjUnlock(virNWFilterObjPtr obj);
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 1ed28a2..c2afdfc 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -566,7 +566,7 @@ nwfilterDefineXML(virConnectPtr conn,
nwfilterDriverLock(driver);
virNWFilterCallbackDriversLock();
- if (!(def = virNWFilterDefParseString(conn, xml)))
+ if (!(def = virNWFilterDefParseString(xml)))
goto cleanup;
if (virNWFilterDefineXMLEnsureACL(conn, def) < 0)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e8bc04d..7376ddb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -612,6 +612,9 @@ qemuStateInitialize(bool privileged,
if (virAsprintf(&driverConf, "%s/qemu.conf", cfg->configBaseDir)
< 0)
goto error;
+ fprintf(stderr, "Start Sleeping to cause a race\n");
+ sleep(10);
+ fprintf(stderr, "Done Sleeping to cause a race\n");
if (virQEMUDriverConfigLoadFile(cfg, driverConf) < 0)
goto error;
VIR_FREE(driverConf);
diff --git a/tests/nwfilterxml2xmltest.c b/tests/nwfilterxml2xmltest.c
index 84e61da..14191a6 100644
--- a/tests/nwfilterxml2xmltest.c
+++ b/tests/nwfilterxml2xmltest.c
@@ -36,7 +36,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml,
virResetLastError();
- if (!(dev = virNWFilterDefParseString(NULL, inXmlData))) {
+ if (!(dev = virNWFilterDefParseString(inXmlData))) {
if (expect_error) {
virResetLastError();
goto done;
--
1.8.3.1
8:41 a.m.
New subject: [libvirt] [PATCH 1/3] Remove virConnectPtr arg from virNWFilterDefParse*
On 10/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
None of the virNWFilterDefParse* methods require a virConnectPtr
arg, so just drop it
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 15 ++++++---------
src/conf/nwfilter_conf.h | 6 ++----
src/nwfilter/nwfilter_driver.c | 2 +-
src/qemu/qemu_driver.c | 3 +++
tests/nwfilterxml2xmltest.c | 2 +-
5 files changed, 13 insertions(+), 15 deletions(-)
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e8bc04d..7376ddb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -612,6 +612,9 @@ qemuStateInitialize(bool privileged,
if (virAsprintf(&driverConf, "%s/qemu.conf", cfg->configBaseDir)
< 0)
goto error;
+ fprintf(stderr, "Start Sleeping to cause a race\n");
+ sleep(10);
+ fprintf(stderr, "Done Sleeping to cause a race\n");
You probably don't want to push this :-)
ACK with that hunk removed.
9:06 a.m.
New subject: [libvirt] [PATCH 1/3] Remove virConnectPtr arg from virNWFilterDefParse*
On Thu, Oct 03, 2013 at 09:41:07AM -0400, Laine Stump wrote:
On 10/03/2013 09:06 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange(a)redhat.com>
>
> None of the virNWFilterDefParse* methods require a virConnectPtr
> arg, so just drop it
>
> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
> ---
> src/conf/nwfilter_conf.c | 15 ++++++---------
> src/conf/nwfilter_conf.h | 6 ++----
> src/nwfilter/nwfilter_driver.c | 2 +-
> src/qemu/qemu_driver.c | 3 +++
> tests/nwfilterxml2xmltest.c | 2 +-
> 5 files changed, 13 insertions(+), 15 deletions(-)
>
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e8bc04d..7376ddb 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -612,6 +612,9 @@ qemuStateInitialize(bool privileged,
> if (virAsprintf(&driverConf, "%s/qemu.conf",
cfg->configBaseDir) < 0)
> goto error;
>
> + fprintf(stderr, "Start Sleeping to cause a race\n");
> + sleep(10);
> + fprintf(stderr, "Done Sleeping to cause a race\n");
You probably don't want to push this :-)
ACK with that hunk removed.
Doh, rebase mistake :-)
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 :|
8:06 a.m.
New subject: [libvirt] [PATCH 2/3] Don't pass virConnectPtr in nwfilter 'struct domUpdateCBStruct'
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The nwfilter driver only needs a reference to its private
state object, not a full virConnectPtr. Update the domUpdateCBStruct
struct to have a 'void *opaque' field instead of a virConnectPtr.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 14 +++++++++++---
src/conf/nwfilter_conf.h | 4 ++--
src/nwfilter/nwfilter_dhcpsnoop.c | 12 ++++++------
src/nwfilter/nwfilter_driver.c | 5 +++--
src/nwfilter/nwfilter_gentech_driver.c | 32 ++++++++++++++++----------------
src/nwfilter/nwfilter_gentech_driver.h | 10 +++++-----
src/nwfilter/nwfilter_learnipaddr.c | 6 +++---
7 files changed, 46 insertions(+), 37 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index c009921..9927f7e 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2850,6 +2850,7 @@ virNWFilterCallbackDriversUnlock(void)
static virDomainObjListIterator virNWFilterDomainFWUpdateCB;
+static void *virNWFilterDomainFWUpdateOpaque;
/**
* virNWFilterInstFiltersOnAllVMs:
@@ -2861,7 +2862,7 @@ virNWFilterInstFiltersOnAllVMs(virConnectPtr conn)
{
size_t i;
struct domUpdateCBStruct cb = {
- .conn = conn,
+ .opaque = virNWFilterDomainFWUpdateOpaque,
.step = STEP_APPLY_CURRENT,
.skipInterfaces = NULL, /* not needed */
};
@@ -2880,7 +2881,7 @@ virNWFilterTriggerVMFilterRebuild(virConnectPtr conn)
size_t i;
int ret = 0;
struct domUpdateCBStruct cb = {
- .conn = conn,
+ .opaque = virNWFilterDomainFWUpdateOpaque,
.step = STEP_APPLY_NEW,
.skipInterfaces = virHashCreate(0, NULL),
};
@@ -3474,9 +3475,14 @@ char *virNWFilterConfigFile(const char *dir,
}
-int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB)
+int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB,
+ void *opaque)
{
+ if (initialized)
+ return -1;
+
virNWFilterDomainFWUpdateCB = domUpdateCB;
+ virNWFilterDomainFWUpdateOpaque = opaque;
initialized = true;
@@ -3495,6 +3501,8 @@ void virNWFilterConfLayerShutdown(void)
virMutexDestroy(&updateMutex);
initialized = false;
+ virNWFilterDomainFWUpdateOpaque = NULL;
+ virNWFilterDomainFWUpdateCB = NULL;
}
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index faa7527..e470615 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -586,7 +586,7 @@ enum UpdateStep {
};
struct domUpdateCBStruct {
- virConnectPtr conn;
+ void *opaque;
enum UpdateStep step;
virHashTablePtr skipInterfaces;
};
@@ -722,7 +722,7 @@ void virNWFilterObjUnlock(virNWFilterObjPtr obj);
void virNWFilterLockFilterUpdates(void);
void virNWFilterUnlockFilterUpdates(void);
-int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB);
+int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque);
void virNWFilterConfLayerShutdown(void);
int virNWFilterInstFiltersOnAllVMs(virConnectPtr conn);
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 3e9f046..2bc1686 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -481,15 +481,15 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
/* instantiate the filters */
if (req->ifname)
- rc = virNWFilterInstantiateFilterLate(NULL,
+ rc = virNWFilterInstantiateFilterLate(req->driver,
+ NULL,
req->ifname,
req->ifindex,
req->linkdev,
req->nettype,
&req->macaddr,
req->filtername,
- req->vars,
- req->driver);
+ req->vars);
exit_snooprequnlock:
virNWFilterSnoopReqUnlock(req);
@@ -867,15 +867,15 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
goto skip_instantiate;
if (ipAddrLeft) {
- ret = virNWFilterInstantiateFilterLate(NULL,
+ ret = virNWFilterInstantiateFilterLate(req->driver,
+ NULL,
req->ifname,
req->ifindex,
req->linkdev,
req->nettype,
&req->macaddr,
req->filtername,
- req->vars,
- req->driver);
+ req->vars);
} else {
const virNWFilterVarValuePtr dhcpsrvrs =
virHashLookup(req->vars->hashTable, NWFILTER_VARNAME_DHCPSERVER);
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index c2afdfc..6e20e03 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -203,7 +203,8 @@ nwfilterStateInitialize(bool privileged,
virNWFilterTechDriversInit(privileged);
- if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB) < 0)
+ if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB,
+ driverState) < 0)
goto err_techdrivers_shutdown;
/*
@@ -681,7 +682,7 @@ nwfilterInstantiateFilter(virConnectPtr conn,
const unsigned char *vmuuid,
virDomainNetDefPtr net)
{
- return virNWFilterInstantiateFilter(conn, vmuuid, net);
+ return virNWFilterInstantiateFilter(conn->nwfilterPrivateData, vmuuid, net);
}
diff --git a/src/nwfilter/nwfilter_gentech_driver.c
b/src/nwfilter/nwfilter_gentech_driver.c
index 382d73f..5961165 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -800,7 +800,8 @@ err_unresolvable_vars:
* Call this function while holding the NWFilter filter update lock
*/
static int
-__virNWFilterInstantiateFilter(const unsigned char *vmuuid,
+__virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
+ const unsigned char *vmuuid,
bool teardownOld,
const char *ifname,
int ifindex,
@@ -810,7 +811,6 @@ __virNWFilterInstantiateFilter(const unsigned char *vmuuid,
const char *filtername,
virNWFilterHashTablePtr filterparams,
enum instCase useNewFilter,
- virNWFilterDriverStatePtr driver,
bool forceWithPendingReq,
bool *foundNewFilter)
{
@@ -921,7 +921,7 @@ err_exit:
static int
-_virNWFilterInstantiateFilter(virConnectPtr conn,
+_virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net,
bool teardownOld,
@@ -948,7 +948,8 @@ _virNWFilterInstantiateFilter(virConnectPtr conn,
goto cleanup;
}
- rc = __virNWFilterInstantiateFilter(vmuuid,
+ rc = __virNWFilterInstantiateFilter(driver,
+ vmuuid,
teardownOld,
net->ifname,
ifindex,
@@ -958,7 +959,6 @@ _virNWFilterInstantiateFilter(virConnectPtr conn,
net->filter,
net->filterparams,
useNewFilter,
- conn->nwfilterPrivateData,
false,
foundNewFilter);
@@ -970,22 +970,23 @@ cleanup:
int
-virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
+virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
+ const unsigned char *vmuuid,
const char *ifname,
int ifindex,
const char *linkdev,
enum virDomainNetType nettype,
const virMacAddrPtr macaddr,
const char *filtername,
- virNWFilterHashTablePtr filterparams,
- virNWFilterDriverStatePtr driver)
+ virNWFilterHashTablePtr filterparams)
{
int rc;
bool foundNewFilter = false;
virNWFilterLockFilterUpdates();
- rc = __virNWFilterInstantiateFilter(vmuuid,
+ rc = __virNWFilterInstantiateFilter(driver,
+ vmuuid,
true,
ifname,
ifindex,
@@ -995,7 +996,6 @@ virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
filtername,
filterparams,
INSTANTIATE_ALWAYS,
- driver,
true,
&foundNewFilter);
if (rc < 0) {
@@ -1015,13 +1015,13 @@ virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
int
-virNWFilterInstantiateFilter(virConnectPtr conn,
+virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net)
{
bool foundNewFilter = false;
- return _virNWFilterInstantiateFilter(conn, vmuuid, net,
+ return _virNWFilterInstantiateFilter(driver, vmuuid, net,
1,
INSTANTIATE_ALWAYS,
&foundNewFilter);
@@ -1029,14 +1029,14 @@ virNWFilterInstantiateFilter(virConnectPtr conn,
int
-virNWFilterUpdateInstantiateFilter(virConnectPtr conn,
+virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net,
bool *skipIface)
{
bool foundNewFilter = false;
- int rc = _virNWFilterInstantiateFilter(conn, vmuuid, net,
+ int rc = _virNWFilterInstantiateFilter(driver, vmuuid, net,
0,
INSTANTIATE_FOLLOW_NEWFILTER,
&foundNewFilter);
@@ -1154,7 +1154,7 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj,
if ((net->filter) && (net->ifname)) {
switch (cb->step) {
case STEP_APPLY_NEW:
- ret = virNWFilterUpdateInstantiateFilter(cb->conn,
+ ret = virNWFilterUpdateInstantiateFilter(cb->opaque,
vm->uuid,
net,
&skipIface);
@@ -1179,7 +1179,7 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj,
break;
case STEP_APPLY_CURRENT:
- ret = virNWFilterInstantiateFilter(cb->conn,
+ ret = virNWFilterInstantiateFilter(cb->opaque,
vm->uuid,
net);
if (ret)
diff --git a/src/nwfilter/nwfilter_gentech_driver.h
b/src/nwfilter/nwfilter_gentech_driver.h
index 4b47b4a..8528e2a 100644
--- a/src/nwfilter/nwfilter_gentech_driver.h
+++ b/src/nwfilter/nwfilter_gentech_driver.h
@@ -39,23 +39,23 @@ enum instCase {
};
-int virNWFilterInstantiateFilter(virConnectPtr conn,
+int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net);
-int virNWFilterUpdateInstantiateFilter(virConnectPtr conn,
+int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net,
bool *skipIface);
-int virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
+int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
+ const unsigned char *vmuuid,
const char *ifname,
int ifindex,
const char *linkdev,
enum virDomainNetType nettype,
const virMacAddrPtr macaddr,
const char *filtername,
- virNWFilterHashTablePtr filterparams,
- virNWFilterDriverStatePtr driver);
+ virNWFilterHashTablePtr filterparams);
int virNWFilterTeardownFilter(const virDomainNetDefPtr net);
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
index 7e67203..093158a 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -612,15 +612,15 @@ learnIPAddressThread(void *arg)
"cache for interface %s"), inetaddr,
req->ifname);
}
- ret = virNWFilterInstantiateFilterLate(NULL,
+ ret = virNWFilterInstantiateFilterLate(req->driver,
+ NULL,
req->ifname,
req->ifindex,
req->linkdev,
req->nettype,
&req->macaddr,
req->filtername,
- req->filterparams,
- req->driver);
+ req->filterparams);
VIR_DEBUG("Result from applying firewall rules on "
"%s with IP addr %s : %d\n", req->ifname, inetaddr,
ret);
}
--
1.8.3.1
9:08 a.m.
New subject: [libvirt] [PATCH 2/3] Don't pass virConnectPtr in nwfilter 'struct domUpdateCBStruct'
On 10/03/2013 07:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The nwfilter driver only needs a reference to its private
state object, not a full virConnectPtr. Update the domUpdateCBStruct
struct to have a 'void *opaque' field instead of a virConnectPtr.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 14 +++++++++++---
src/conf/nwfilter_conf.h | 4 ++--
src/nwfilter/nwfilter_dhcpsnoop.c | 12 ++++++------
src/nwfilter/nwfilter_driver.c | 5 +++--
src/nwfilter/nwfilter_gentech_driver.c | 32 ++++++++++++++++----------------
src/nwfilter/nwfilter_gentech_driver.h | 10 +++++-----
src/nwfilter/nwfilter_learnipaddr.c | 6 +++---
7 files changed, 46 insertions(+), 37 deletions(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:13 a.m.
New subject: [libvirt] [PATCH 2/3] Don't pass virConnectPtr in nwfilter 'struct domUpdateCBStruct'
On 10/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The nwfilter driver only needs a reference to its private
state object, not a full virConnectPtr. Update the domUpdateCBStruct
struct to have a 'void *opaque' field instead of a virConnectPtr.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 14 +++++++++++---
src/conf/nwfilter_conf.h | 4 ++--
src/nwfilter/nwfilter_dhcpsnoop.c | 12 ++++++------
src/nwfilter/nwfilter_driver.c | 5 +++--
src/nwfilter/nwfilter_gentech_driver.c | 32 ++++++++++++++++----------------
src/nwfilter/nwfilter_gentech_driver.h | 10 +++++-----
src/nwfilter/nwfilter_learnipaddr.c | 6 +++---
7 files changed, 46 insertions(+), 37 deletions(-)
I did a sanity compile / syntax-check, and manually looked through the
code and it all looks okay. So ACK based on that (I'm assuming that
you've actually fired it all up :-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index c009921..9927f7e 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2850,6 +2850,7 @@ virNWFilterCallbackDriversUnlock(void)
static virDomainObjListIterator virNWFilterDomainFWUpdateCB;
+static void *virNWFilterDomainFWUpdateOpaque;
/**
* virNWFilterInstFiltersOnAllVMs:
@@ -2861,7 +2862,7 @@ virNWFilterInstFiltersOnAllVMs(virConnectPtr conn)
{
size_t i;
struct domUpdateCBStruct cb = {
- .conn = conn,
+ .opaque = virNWFilterDomainFWUpdateOpaque,
.step = STEP_APPLY_CURRENT,
.skipInterfaces = NULL, /* not needed */
};
@@ -2880,7 +2881,7 @@ virNWFilterTriggerVMFilterRebuild(virConnectPtr conn)
size_t i;
int ret = 0;
struct domUpdateCBStruct cb = {
- .conn = conn,
+ .opaque = virNWFilterDomainFWUpdateOpaque,
.step = STEP_APPLY_NEW,
.skipInterfaces = virHashCreate(0, NULL),
};
@@ -3474,9 +3475,14 @@ char *virNWFilterConfigFile(const char *dir,
}
-int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB)
+int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB,
+ void *opaque)
{
+ if (initialized)
+ return -1;
+
virNWFilterDomainFWUpdateCB = domUpdateCB;
+ virNWFilterDomainFWUpdateOpaque = opaque;
initialized = true;
@@ -3495,6 +3501,8 @@ void virNWFilterConfLayerShutdown(void)
virMutexDestroy(&updateMutex);
initialized = false;
+ virNWFilterDomainFWUpdateOpaque = NULL;
+ virNWFilterDomainFWUpdateCB = NULL;
}
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index faa7527..e470615 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -586,7 +586,7 @@ enum UpdateStep {
};
struct domUpdateCBStruct {
- virConnectPtr conn;
+ void *opaque;
enum UpdateStep step;
virHashTablePtr skipInterfaces;
};
@@ -722,7 +722,7 @@ void virNWFilterObjUnlock(virNWFilterObjPtr obj);
void virNWFilterLockFilterUpdates(void);
void virNWFilterUnlockFilterUpdates(void);
-int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB);
+int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque);
void virNWFilterConfLayerShutdown(void);
int virNWFilterInstFiltersOnAllVMs(virConnectPtr conn);
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 3e9f046..2bc1686 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -481,15 +481,15 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
/* instantiate the filters */
if (req->ifname)
- rc = virNWFilterInstantiateFilterLate(NULL,
+ rc = virNWFilterInstantiateFilterLate(req->driver,
+ NULL,
req->ifname,
req->ifindex,
req->linkdev,
req->nettype,
&req->macaddr,
req->filtername,
- req->vars,
- req->driver);
+ req->vars);
exit_snooprequnlock:
virNWFilterSnoopReqUnlock(req);
@@ -867,15 +867,15 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
goto skip_instantiate;
if (ipAddrLeft) {
- ret = virNWFilterInstantiateFilterLate(NULL,
+ ret = virNWFilterInstantiateFilterLate(req->driver,
+ NULL,
req->ifname,
req->ifindex,
req->linkdev,
req->nettype,
&req->macaddr,
req->filtername,
- req->vars,
- req->driver);
+ req->vars);
} else {
const virNWFilterVarValuePtr dhcpsrvrs =
virHashLookup(req->vars->hashTable, NWFILTER_VARNAME_DHCPSERVER);
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index c2afdfc..6e20e03 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -203,7 +203,8 @@ nwfilterStateInitialize(bool privileged,
virNWFilterTechDriversInit(privileged);
- if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB) < 0)
+ if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB,
+ driverState) < 0)
goto err_techdrivers_shutdown;
/*
@@ -681,7 +682,7 @@ nwfilterInstantiateFilter(virConnectPtr conn,
const unsigned char *vmuuid,
virDomainNetDefPtr net)
{
- return virNWFilterInstantiateFilter(conn, vmuuid, net);
+ return virNWFilterInstantiateFilter(conn->nwfilterPrivateData, vmuuid, net);
}
diff --git a/src/nwfilter/nwfilter_gentech_driver.c
b/src/nwfilter/nwfilter_gentech_driver.c
index 382d73f..5961165 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -800,7 +800,8 @@ err_unresolvable_vars:
* Call this function while holding the NWFilter filter update lock
*/
static int
-__virNWFilterInstantiateFilter(const unsigned char *vmuuid,
+__virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
+ const unsigned char *vmuuid,
bool teardownOld,
const char *ifname,
int ifindex,
@@ -810,7 +811,6 @@ __virNWFilterInstantiateFilter(const unsigned char *vmuuid,
const char *filtername,
virNWFilterHashTablePtr filterparams,
enum instCase useNewFilter,
- virNWFilterDriverStatePtr driver,
bool forceWithPendingReq,
bool *foundNewFilter)
{
@@ -921,7 +921,7 @@ err_exit:
static int
-_virNWFilterInstantiateFilter(virConnectPtr conn,
+_virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net,
bool teardownOld,
@@ -948,7 +948,8 @@ _virNWFilterInstantiateFilter(virConnectPtr conn,
goto cleanup;
}
- rc = __virNWFilterInstantiateFilter(vmuuid,
+ rc = __virNWFilterInstantiateFilter(driver,
+ vmuuid,
teardownOld,
net->ifname,
ifindex,
@@ -958,7 +959,6 @@ _virNWFilterInstantiateFilter(virConnectPtr conn,
net->filter,
net->filterparams,
useNewFilter,
- conn->nwfilterPrivateData,
false,
foundNewFilter);
@@ -970,22 +970,23 @@ cleanup:
int
-virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
+virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
+ const unsigned char *vmuuid,
const char *ifname,
int ifindex,
const char *linkdev,
enum virDomainNetType nettype,
const virMacAddrPtr macaddr,
const char *filtername,
- virNWFilterHashTablePtr filterparams,
- virNWFilterDriverStatePtr driver)
+ virNWFilterHashTablePtr filterparams)
{
int rc;
bool foundNewFilter = false;
virNWFilterLockFilterUpdates();
- rc = __virNWFilterInstantiateFilter(vmuuid,
+ rc = __virNWFilterInstantiateFilter(driver,
+ vmuuid,
true,
ifname,
ifindex,
@@ -995,7 +996,6 @@ virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
filtername,
filterparams,
INSTANTIATE_ALWAYS,
- driver,
true,
&foundNewFilter);
if (rc < 0) {
@@ -1015,13 +1015,13 @@ virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
int
-virNWFilterInstantiateFilter(virConnectPtr conn,
+virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net)
{
bool foundNewFilter = false;
- return _virNWFilterInstantiateFilter(conn, vmuuid, net,
+ return _virNWFilterInstantiateFilter(driver, vmuuid, net,
1,
INSTANTIATE_ALWAYS,
&foundNewFilter);
@@ -1029,14 +1029,14 @@ virNWFilterInstantiateFilter(virConnectPtr conn,
int
-virNWFilterUpdateInstantiateFilter(virConnectPtr conn,
+virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net,
bool *skipIface)
{
bool foundNewFilter = false;
- int rc = _virNWFilterInstantiateFilter(conn, vmuuid, net,
+ int rc = _virNWFilterInstantiateFilter(driver, vmuuid, net,
0,
INSTANTIATE_FOLLOW_NEWFILTER,
&foundNewFilter);
@@ -1154,7 +1154,7 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj,
if ((net->filter) && (net->ifname)) {
switch (cb->step) {
case STEP_APPLY_NEW:
- ret = virNWFilterUpdateInstantiateFilter(cb->conn,
+ ret = virNWFilterUpdateInstantiateFilter(cb->opaque,
vm->uuid,
net,
&skipIface);
@@ -1179,7 +1179,7 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj,
break;
case STEP_APPLY_CURRENT:
- ret = virNWFilterInstantiateFilter(cb->conn,
+ ret = virNWFilterInstantiateFilter(cb->opaque,
vm->uuid,
net);
if (ret)
diff --git a/src/nwfilter/nwfilter_gentech_driver.h
b/src/nwfilter/nwfilter_gentech_driver.h
index 4b47b4a..8528e2a 100644
--- a/src/nwfilter/nwfilter_gentech_driver.h
+++ b/src/nwfilter/nwfilter_gentech_driver.h
@@ -39,23 +39,23 @@ enum instCase {
};
-int virNWFilterInstantiateFilter(virConnectPtr conn,
+int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net);
-int virNWFilterUpdateInstantiateFilter(virConnectPtr conn,
+int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver,
const unsigned char *vmuuid,
const virDomainNetDefPtr net,
bool *skipIface);
-int virNWFilterInstantiateFilterLate(const unsigned char *vmuuid,
+int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
+ const unsigned char *vmuuid,
const char *ifname,
int ifindex,
const char *linkdev,
enum virDomainNetType nettype,
const virMacAddrPtr macaddr,
const char *filtername,
- virNWFilterHashTablePtr filterparams,
- virNWFilterDriverStatePtr driver);
+ virNWFilterHashTablePtr filterparams);
int virNWFilterTeardownFilter(const virDomainNetDefPtr net);
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
index 7e67203..093158a 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -612,15 +612,15 @@ learnIPAddressThread(void *arg)
"cache for interface %s"), inetaddr,
req->ifname);
}
- ret = virNWFilterInstantiateFilterLate(NULL,
+ ret = virNWFilterInstantiateFilterLate(req->driver,
+ NULL,
req->ifname,
req->ifindex,
req->linkdev,
req->nettype,
&req->macaddr,
req->filtername,
- req->filterparams,
- req->driver);
+ req->filterparams);
VIR_DEBUG("Result from applying firewall rules on "
"%s with IP addr %s : %d\n", req->ifname, inetaddr,
ret);
}
8:07 a.m.
New subject: [libvirt] [PATCH 3/3] Remove use of virConnectPtr from all remaining nwfilter code
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The virConnectPtr is passed around loads of nwfilter code in
order to provide it as a parameter to the callback registered
by the virt drivers. None of the virt drivers use this param
though, so it serves no purpose.
Avoiding the need to pass a virConnectPtr means that the
nwfilterStateReload method no longer needs to open a bogus
QEMU driver connection. This addresses a race condition that
can lead to a crash on startup.
The nwfilter driver starts before the QEMU driver and registers
some callbacks with DBus to detect firewalld reload. If the
firewalld reload happens while the QEMU driver is still starting
up though, the nwfilterStateReload method will open a connection
to the partially initialized QEMU driver and cause a crash.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 49 ++++++++++++++++--------------------------
src/conf/nwfilter_conf.h | 14 +++++-------
src/lxc/lxc_driver.c | 3 +--
src/nwfilter/nwfilter_driver.c | 42 ++++++++++++++----------------------
src/qemu/qemu_driver.c | 3 +--
src/uml/uml_driver.c | 3 +--
6 files changed, 43 insertions(+), 71 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 9927f7e..7152aae 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2744,8 +2744,7 @@ cleanup:
static int
-_virNWFilterDefLoopDetect(virConnectPtr conn,
- virNWFilterObjListPtr nwfilters,
+_virNWFilterDefLoopDetect(virNWFilterObjListPtr nwfilters,
virNWFilterDefPtr def,
const char *filtername)
{
@@ -2769,7 +2768,7 @@ _virNWFilterDefLoopDetect(virConnectPtr conn,
obj = virNWFilterObjFindByName(nwfilters,
entry->include->filterref);
if (obj) {
- rc = _virNWFilterDefLoopDetect(conn, nwfilters,
+ rc = _virNWFilterDefLoopDetect(nwfilters,
obj->def, filtername);
virNWFilterObjUnlock(obj);
@@ -2785,7 +2784,6 @@ _virNWFilterDefLoopDetect(virConnectPtr conn,
/*
* virNWFilterDefLoopDetect:
- * @conn: pointer to virConnect object
* @nwfilters : the nwfilters to search
* @def : the filter definition that may add a loop and is to be tested
*
@@ -2795,11 +2793,10 @@ _virNWFilterDefLoopDetect(virConnectPtr conn,
* Returns 0 in case no loop was detected, -1 otherwise.
*/
static int
-virNWFilterDefLoopDetect(virConnectPtr conn,
- virNWFilterObjListPtr nwfilters,
+virNWFilterDefLoopDetect(virNWFilterObjListPtr nwfilters,
virNWFilterDefPtr def)
{
- return _virNWFilterDefLoopDetect(conn, nwfilters, def, def->name);
+ return _virNWFilterDefLoopDetect(nwfilters, def, def->name);
}
int nCallbackDriver;
@@ -2858,7 +2855,7 @@ static void *virNWFilterDomainFWUpdateOpaque;
* error. This should be called upon reloading of the driver.
*/
int
-virNWFilterInstFiltersOnAllVMs(virConnectPtr conn)
+virNWFilterInstFiltersOnAllVMs(void)
{
size_t i;
struct domUpdateCBStruct cb = {
@@ -2868,15 +2865,14 @@ virNWFilterInstFiltersOnAllVMs(virConnectPtr conn)
};
for (i = 0; i < nCallbackDriver; i++)
- callbackDrvArray[i]->vmFilterRebuild(conn,
- virNWFilterDomainFWUpdateCB,
+ callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB,
&cb);
return 0;
}
static int
-virNWFilterTriggerVMFilterRebuild(virConnectPtr conn)
+virNWFilterTriggerVMFilterRebuild(void)
{
size_t i;
int ret = 0;
@@ -2890,8 +2886,7 @@ virNWFilterTriggerVMFilterRebuild(virConnectPtr conn)
return -1;
for (i = 0; i < nCallbackDriver; i++) {
- if (callbackDrvArray[i]->vmFilterRebuild(conn,
- virNWFilterDomainFWUpdateCB,
+ if (callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB,
&cb) < 0)
ret = -1;
}
@@ -2900,15 +2895,13 @@ virNWFilterTriggerVMFilterRebuild(virConnectPtr conn)
cb.step = STEP_TEAR_NEW; /* rollback */
for (i = 0; i < nCallbackDriver; i++)
- callbackDrvArray[i]->vmFilterRebuild(conn,
- virNWFilterDomainFWUpdateCB,
+ callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB,
&cb);
} else {
cb.step = STEP_TEAR_OLD; /* switch over */
for (i = 0; i < nCallbackDriver; i++)
- callbackDrvArray[i]->vmFilterRebuild(conn,
- virNWFilterDomainFWUpdateCB,
+ callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB,
&cb);
}
@@ -2919,14 +2912,13 @@ virNWFilterTriggerVMFilterRebuild(virConnectPtr conn)
int
-virNWFilterTestUnassignDef(virConnectPtr conn,
- virNWFilterObjPtr nwfilter)
+virNWFilterTestUnassignDef(virNWFilterObjPtr nwfilter)
{
int rc = 0;
nwfilter->wantRemoved = 1;
/* trigger the update on VMs referencing the filter */
- if (virNWFilterTriggerVMFilterRebuild(conn))
+ if (virNWFilterTriggerVMFilterRebuild())
rc = -1;
nwfilter->wantRemoved = 0;
@@ -2965,8 +2957,7 @@ cleanup:
}
virNWFilterObjPtr
-virNWFilterObjAssignDef(virConnectPtr conn,
- virNWFilterObjListPtr nwfilters,
+virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
virNWFilterDefPtr def)
{
virNWFilterObjPtr nwfilter;
@@ -2985,7 +2976,7 @@ virNWFilterObjAssignDef(virConnectPtr conn,
virNWFilterObjUnlock(nwfilter);
}
- if (virNWFilterDefLoopDetect(conn, nwfilters, def) < 0) {
+ if (virNWFilterDefLoopDetect(nwfilters, def) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("filter would introduce a loop"));
return NULL;
@@ -3004,7 +2995,7 @@ virNWFilterObjAssignDef(virConnectPtr conn,
nwfilter->newDef = def;
/* trigger the update on VMs referencing the filter */
- if (virNWFilterTriggerVMFilterRebuild(conn)) {
+ if (virNWFilterTriggerVMFilterRebuild()) {
nwfilter->newDef = NULL;
virNWFilterUnlockFilterUpdates();
virNWFilterObjUnlock(nwfilter);
@@ -3046,8 +3037,7 @@ virNWFilterObjAssignDef(virConnectPtr conn,
static virNWFilterObjPtr
-virNWFilterObjLoad(virConnectPtr conn,
- virNWFilterObjListPtr nwfilters,
+virNWFilterObjLoad(virNWFilterObjListPtr nwfilters,
const char *file,
const char *path)
{
@@ -3066,7 +3056,7 @@ virNWFilterObjLoad(virConnectPtr conn,
return NULL;
}
- if (!(nwfilter = virNWFilterObjAssignDef(conn, nwfilters, def))) {
+ if (!(nwfilter = virNWFilterObjAssignDef(nwfilters, def))) {
virNWFilterDefFree(def);
return NULL;
}
@@ -3082,8 +3072,7 @@ virNWFilterObjLoad(virConnectPtr conn,
int
-virNWFilterLoadAllConfigs(virConnectPtr conn,
- virNWFilterObjListPtr nwfilters,
+virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters,
const char *configDir)
{
DIR *dir;
@@ -3111,7 +3100,7 @@ virNWFilterLoadAllConfigs(virConnectPtr conn,
if (!(path = virFileBuildPath(configDir, entry->d_name, NULL)))
continue;
- nwfilter = virNWFilterObjLoad(conn, nwfilters, entry->d_name, path);
+ nwfilter = virNWFilterObjLoad(nwfilters, entry->d_name, path);
if (nwfilter)
virNWFilterObjUnlock(nwfilter);
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index e470615..29906f1 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -687,12 +687,10 @@ int virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver,
int virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter);
-virNWFilterObjPtr virNWFilterObjAssignDef(virConnectPtr conn,
- virNWFilterObjListPtr nwfilters,
+virNWFilterObjPtr virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
virNWFilterDefPtr def);
-int virNWFilterTestUnassignDef(virConnectPtr conn,
- virNWFilterObjPtr nwfilter);
+int virNWFilterTestUnassignDef(virNWFilterObjPtr nwfilter);
virNWFilterDefPtr virNWFilterDefParseNode(xmlDocPtr xml,
xmlNodePtr root);
@@ -706,8 +704,7 @@ int virNWFilterSaveXML(const char *configDir,
int virNWFilterSaveConfig(const char *configDir,
virNWFilterDefPtr def);
-int virNWFilterLoadAllConfigs(virConnectPtr conn,
- virNWFilterObjListPtr nwfilters,
+int virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters,
const char *configDir);
char *virNWFilterConfigFile(const char *dir,
@@ -725,11 +722,10 @@ void virNWFilterUnlockFilterUpdates(void);
int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque);
void virNWFilterConfLayerShutdown(void);
-int virNWFilterInstFiltersOnAllVMs(virConnectPtr conn);
+int virNWFilterInstFiltersOnAllVMs(void);
-typedef int (*virNWFilterRebuild)(virConnectPtr conn,
- virDomainObjListIterator domUpdateCB,
+typedef int (*virNWFilterRebuild)(virDomainObjListIterator domUpdateCB,
void *data);
typedef void (*virNWFilterVoidCall)(void);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 8b13f84..e3a34d6 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -84,8 +84,7 @@ virLXCDriverPtr lxc_driver = NULL;
/* callbacks for nwfilter */
static int
-lxcVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
- virDomainObjListIterator iter, void *data)
+lxcVMFilterRebuild(virDomainObjListIterator iter, void *data)
{
return virDomainObjListForEach(lxc_driver->domains, iter, data);
}
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 6e20e03..d25c6f2 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -235,8 +235,7 @@ nwfilterStateInitialize(bool privileged,
VIR_FREE(base);
- if (virNWFilterLoadAllConfigs(NULL,
- &driverState->nwfilters,
+ if (virNWFilterLoadAllConfigs(&driverState->nwfilters,
driverState->configDir) < 0)
goto error;
@@ -272,37 +271,28 @@ err_free_driverstate:
* files and update its state
*/
static int
-nwfilterStateReload(void) {
- virConnectPtr conn;
-
- if (!driverState) {
+nwfilterStateReload(void)
+{
+ if (!driverState)
return -1;
- }
if (!driverState->privileged)
return 0;
- conn = virConnectOpen("qemu:///system");
-
- if (conn) {
- virNWFilterDHCPSnoopEnd(NULL);
- /* shut down all threads -- they will be restarted if necessary */
- virNWFilterLearnThreadsTerminate(true);
-
- nwfilterDriverLock(driverState);
- virNWFilterCallbackDriversLock();
+ virNWFilterDHCPSnoopEnd(NULL);
+ /* shut down all threads -- they will be restarted if necessary */
+ virNWFilterLearnThreadsTerminate(true);
- virNWFilterLoadAllConfigs(conn,
- &driverState->nwfilters,
- driverState->configDir);
+ nwfilterDriverLock(driverState);
+ virNWFilterCallbackDriversLock();
- virNWFilterCallbackDriversUnlock();
- nwfilterDriverUnlock(driverState);
+ virNWFilterLoadAllConfigs(&driverState->nwfilters,
+ driverState->configDir);
- virNWFilterInstFiltersOnAllVMs(conn);
+ virNWFilterCallbackDriversUnlock();
+ nwfilterDriverUnlock(driverState);
- virConnectClose(conn);
- }
+ virNWFilterInstFiltersOnAllVMs();
return 0;
}
@@ -573,7 +563,7 @@ nwfilterDefineXML(virConnectPtr conn,
if (virNWFilterDefineXMLEnsureACL(conn, def) < 0)
goto cleanup;
- if (!(nwfilter = virNWFilterObjAssignDef(conn, &driver->nwfilters, def)))
+ if (!(nwfilter = virNWFilterObjAssignDef(&driver->nwfilters, def)))
goto cleanup;
if (virNWFilterObjSaveDef(driver, nwfilter, def) < 0) {
@@ -617,7 +607,7 @@ nwfilterUndefine(virNWFilterPtr obj) {
if (virNWFilterUndefineEnsureACL(obj->conn, nwfilter->def) < 0)
goto cleanup;
- if (virNWFilterTestUnassignDef(obj->conn, nwfilter) < 0) {
+ if (virNWFilterTestUnassignDef(nwfilter) < 0) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s",
_("nwfilter is in use"));
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7376ddb..ae49b08 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -177,8 +177,7 @@ static void
qemuVMDriverUnlock(void) {}
static int
-qemuVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
- virDomainObjListIterator iter, void *data)
+qemuVMFilterRebuild(virDomainObjListIterator iter, void *data)
{
return virDomainObjListForEach(qemu_driver->domains, iter, data);
}
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 9ca352f..eb02542 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -148,8 +148,7 @@ static int umlMonitorCommand(const struct uml_driver *driver,
static struct uml_driver *uml_driver = NULL;
static int
-umlVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
- virDomainObjListIterator iter, void *data)
+umlVMFilterRebuild(virDomainObjListIterator iter, void *data)
{
return virDomainObjListForEach(uml_driver->domains, iter, data);
}
--
1.8.3.1
8:52 a.m.
New subject: [libvirt] [PATCH 3/3] Remove use of virConnectPtr from all remaining nwfilter code
On 10/03/2013 09:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The virConnectPtr is passed around loads of nwfilter code in
order to provide it as a parameter to the callback registered
by the virt drivers. None of the virt drivers use this param
though, so it serves no purpose.
Avoiding the need to pass a virConnectPtr means that the
nwfilterStateReload method no longer needs to open a bogus
QEMU driver connection. This addresses a race condition that
can lead to a crash on startup.
The nwfilter driver starts before the QEMU driver and registers
some callbacks with DBus to detect firewalld reload. If the
firewalld reload happens while the QEMU driver is still starting
up though, the nwfilterStateReload method will open a connection
to the partially initialized QEMU driver and cause a crash.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 49 ++++++++++++++++--------------------------
src/conf/nwfilter_conf.h | 14 +++++-------
src/lxc/lxc_driver.c | 3 +--
src/nwfilter/nwfilter_driver.c | 42 ++++++++++++++----------------------
src/qemu/qemu_driver.c | 3 +--
src/uml/uml_driver.c | 3 +--
6 files changed, 43 insertions(+), 71 deletions(-)
ACK.
4069
days inactive
4069
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump