[libvirt] [PATCH 0/3] Avoid crash due to race in nwfilter reload/libvirtd startup
From: "Daniel P. Berrange" <berrange@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
From: "Daniel P. Berrange" <berrange@redhat.com> None of the virNWFilterDefParse* methods require a virConnectPtr arg, so just drop it Signed-off-by: Daniel P. Berrange <berrange@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
On 10/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
None of the virNWFilterDefParse* methods require a virConnectPtr arg, so just drop it
Signed-off-by: Daniel P. Berrange <berrange@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.
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@redhat.com>
None of the virNWFilterDefParse* methods require a virConnectPtr arg, so just drop it
Signed-off-by: Daniel P. Berrange <berrange@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 :|
From: "Daniel P. Berrange" <berrange@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@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
On 10/03/2013 07:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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
On 10/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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); }
From: "Daniel P. Berrange" <berrange@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@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
On 10/03/2013 09:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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.
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump