[libvirt] [PATCH] [RFC] nwfilter: fix loadable module support
Hello! I am trying to fix the loadable modules support by removing all direct calls from qemu_driver.c and qemu_conf.c into nwfilter functions. The below patch extends the nwfilter driver interface with 2 more private functions to instantiate and tear down the filters. I call them 'private' because no client should ever be calling them via RPC and be able to tear down the filters for example -- they should only ever be callable from within libvirtd. I extended the qemudShutdownVMDaemon function with the virConnectPtr type of parameter so that I have the pointer to conn->nwfilterDriver to access the private nwfilter driver functions. The problem with that is that the conn pointer is not always available to be passed (qemuReconnectDomain, qemuHandleMonitorEOF), particularly in tear-down functions, and so I have to pass a NULL pointer, which then prevents the calling of the private interface function to tear down the filters.Does anyone have a suggestion on how I would best change the interface with the nwfilter driver to fix this particular problem? Stefan Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/esx/esx_nwfilter_driver.c | 10 ++++++++ src/nwfilter/nwfilter_driver.c | 15 ++++++++++++ src/nwfilter/nwfilter_gentech_driver.h | 26 ++++++++++++++++----- src/qemu/qemu_conf.c | 10 ++++---- src/qemu/qemu_driver.c | 40 ++++++++++++++++++--------------- 5 files changed, 72 insertions(+), 29 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -410,6 +410,19 @@ cleanup: } +static int +nwfilterInstantiateFilter(virConnectPtr conn, + virDomainNetDefPtr net) { + return virNWFilterInstantiateFilter(conn, net); +} + + +static void +nwfilterTeardownFilter(virDomainNetDefPtr net) { + virNWFilterTeardownFilter(net); +} + + static virNWFilterDriver nwfilterDriver = { .name = "nwfilter", .open = nwfilterOpen, @@ -421,6 +434,8 @@ static virNWFilterDriver nwfilterDriver .defineXML = nwfilterDefine, .undefine = nwfilterUndefine, .getXMLDesc = nwfilterDumpXML, + .privateInstantiateFilter = nwfilterInstantiateFilter, + .privateTeardownFilter = nwfilterTeardownFilter, }; Index: libvirt-acl/src/esx/esx_nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/esx/esx_nwfilter_driver.c +++ libvirt-acl/src/esx/esx_nwfilter_driver.c @@ -75,6 +75,8 @@ static virNWFilterDriver esxNWFilterDriv NULL, /* defineXML */ NULL, /* undefine */ NULL, /* getXMLDesc */ + NULL, /* privateInstantiateFilter */ + NULL, /* privateTeardownFilter */ }; Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h @@ -23,6 +23,8 @@ #ifndef __NWFILTER_GENTECH_DRIVER_H # define __NWFILTER_GENTECH_DRIVER_H +# include "datatypes.h" + virNWFilterTechDriverPtr virNWFilterTechDriverForName(const char *name); int virNWFilterRuleInstAddData(virNWFilterRuleInstPtr res, @@ -37,8 +39,18 @@ enum instCase { }; +static inline +int virNWFilterInstNWFilter(virConnectPtr conn, + const virDomainNetDefPtr net) +{ + if ((conn) && (conn->nwfilterDriver)) + return conn->nwfilterDriver->privateInstantiateFilter(conn, net); + return 0; +} + int virNWFilterInstantiateFilter(virConnectPtr conn, const virDomainNetDefPtr net); + int virNWFilterUpdateInstantiateFilter(virConnectPtr conn, const virDomainNetDefPtr net, bool *skipIface); @@ -70,18 +82,20 @@ void virNWFilterDomainFWUpdateCB(void *p /* tear down an interface's filter before tearing down the interface */ static inline void -virNWFilterTearNWFilter(virDomainNetDefPtr net) { - if ((net->filter) && (net->ifname)) - virNWFilterTeardownFilter(net); +virNWFilterTearNWFilter(virConnectPtr conn, virDomainNetDefPtr net) { + if ((conn) && (conn->nwfilterDriver) && (net->filter) && (net->ifname)) + conn->nwfilterDriver->privateTeardownFilter(net); } static inline void -virNWFilterTearVMNWFilters(virDomainObjPtr vm) { +virNWFilterTearVMNWFilters(virConnectPtr conn, virDomainObjPtr vm) { int i; - for (i = 0; i < vm->def->nnets; i++) - virNWFilterTearNWFilter(vm->def->nets[i]); + if (conn) { + for (i = 0; i < vm->def->nnets; i++) + virNWFilterTearNWFilter(conn, vm->def->nets[i]); + } } #endif Index: libvirt-acl/src/qemu/qemu_conf.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_conf.c +++ libvirt-acl/src/qemu/qemu_conf.c @@ -1555,7 +1555,7 @@ qemudPhysIfaceConnect(virConnectPtr conn if (rc >= 0) { if ((net->filter) && (net->ifname)) { - err = virNWFilterInstantiateFilter(conn, net); + err = virNWFilterInstNWFilter(conn, net); if (err) { close(rc); rc = -1; @@ -1688,7 +1688,7 @@ qemudNetworkIfaceConnect(virConnectPtr c if (tapfd >= 0) { if ((net->filter) && (net->ifname)) { - err = virNWFilterInstantiateFilter(conn, net); + err = virNWFilterInstNWFilter(conn, net); if (err) { close(tapfd); tapfd = -1; @@ -4207,7 +4207,7 @@ int qemudBuildCommandLine(virConnectPtr goto error; if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { - virNWFilterTearNWFilter(net); + virNWFilterTearNWFilter(conn, net); close(tapfd); goto no_memory; } @@ -4226,7 +4226,7 @@ int qemudBuildCommandLine(virConnectPtr goto error; if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { - virNWFilterTearNWFilter(net); + virNWFilterTearNWFilter(conn, net); close(tapfd); goto no_memory; } @@ -4766,7 +4766,7 @@ int qemudBuildCommandLine(virConnectPtr virReportOOMError(); error: for (i = 0; i <= last_good_net; i++) - virNWFilterTearNWFilter(def->nets[i]); + virNWFilterTearNWFilter(conn, def->nets[i]); if (vmfds && *vmfds) { for (i = 0; i < *nvmfds; i++) Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -158,7 +158,8 @@ static int qemudStartVMDaemon(virConnect int stdin_fd, const char *stdin_path); -static void qemudShutdownVMDaemon(struct qemud_driver *driver, +static void qemudShutdownVMDaemon(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, int migrated); @@ -735,7 +736,7 @@ qemuHandleMonitorEOF(qemuMonitorPtr mon VIR_DOMAIN_EVENT_STOPPED_FAILED : VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(NULL, driver, vm, 0); if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); else @@ -1283,7 +1284,7 @@ error: /* We can't get the monitor back, so must kill the VM * to remove danger of it ending up running twice if * user tries to start it again later */ - qemudShutdownVMDaemon(driver, obj, 0); + qemudShutdownVMDaemon(NULL, driver, obj, 0); if (!obj->persistent) virDomainRemoveInactive(&driver->domains, obj); else @@ -3576,7 +3577,7 @@ static int qemudStartVMDaemon(virConnect VIR_FREE(progenv); if (ret == -1) /* The VM failed to start; tear filters before taps */ - virNWFilterTearVMNWFilters(vm); + virNWFilterTearVMNWFilters(conn, vm); if (vmfds) { for (i = 0 ; i < nvmfds ; i++) { @@ -3642,7 +3643,7 @@ cleanup: /* We jump here if we failed to start the VM for any reason, or * if we failed to initialize the now running VM. kill it off and * pretend we never started it */ - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(conn, driver, vm, 0); if (logfile != -1) close(logfile); @@ -3651,7 +3652,8 @@ cleanup: } -static void qemudShutdownVMDaemon(struct qemud_driver *driver, +static void qemudShutdownVMDaemon(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, int migrated) { int ret; @@ -3668,7 +3670,7 @@ static void qemudShutdownVMDaemon(struct * reporting so we don't squash a legit error. */ orig_err = virSaveLastError(); - virNWFilterTearVMNWFilters(vm); + virNWFilterTearVMNWFilters(conn, vm); if (driver->macFilter) { def = vm->def; @@ -4465,7 +4467,7 @@ static int qemudDomainDestroy(virDomainP goto endjob; } - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(dom->conn, driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); @@ -5211,7 +5213,7 @@ static int qemudDomainSaveFlag(virDomain ret = 0; /* Shut it down */ - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(dom->conn, driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); @@ -5528,7 +5530,7 @@ static int qemudDomainCoreDump(virDomain endjob: if ((ret == 0) && (flags & VIR_DUMP_CRASH)) { - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(dom->conn, driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); @@ -7640,7 +7642,7 @@ cleanup: VIR_WARN0("Unable to release PCI address on NIC"); if (ret != 0) - virNWFilterTearNWFilter(net); + virNWFilterTearNWFilter(conn, net); VIR_FREE(nicstr); VIR_FREE(netstr); @@ -8534,7 +8536,8 @@ cleanup: } static int -qemudDomainDetachNetDevice(struct qemud_driver *driver, +qemudDomainDetachNetDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev, unsigned long long qemuCmdFlags) @@ -8609,7 +8612,7 @@ qemudDomainDetachNetDevice(struct qemud_ } qemuDomainObjExitMonitorWithDriver(driver, vm); - virNWFilterTearNWFilter(detach); + virNWFilterTearNWFilter(conn, detach); #if WITH_MACVTAP if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) { @@ -8921,7 +8924,8 @@ static int qemudDomainDetachDevice(virDo _("This type of disk cannot be hot unplugged")); } } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { - ret = qemudDomainDetachNetDevice(driver, vm, dev, qemuCmdFlags); + ret = qemudDomainDetachNetDevice(dom->conn, driver, vm, dev, + qemuCmdFlags); } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { if (dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { ret = qemudDomainDetachPciControllerDevice(driver, vm, dev, @@ -10215,7 +10219,7 @@ qemudDomainMigratePrepareTunnel(virConne qemust = qemuStreamMigOpen(st, unixfile); if (qemust == NULL) { - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(dconn, driver, vm, 0); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); @@ -10940,7 +10944,7 @@ qemudDomainMigratePerform (virDomainPtr } /* Clean up the source domain. */ - qemudShutdownVMDaemon(driver, vm, 1); + qemudShutdownVMDaemon(dom->conn, driver, vm, 1); resume = 0; event = virDomainEventNewFromObj(vm, @@ -11100,7 +11104,7 @@ qemudDomainMigrateFinish2 (virConnectPtr goto endjob; } } else { - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(dconn, driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); @@ -11946,7 +11950,7 @@ static int qemuDomainRevertToSnapshot(vi */ if (virDomainObjIsActive(vm)) { - qemudShutdownVMDaemon(driver, vm, 0); + qemudShutdownVMDaemon(snapshot->domain->conn, driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); Index: libvirt-acl/src/driver.h =================================================================== --- libvirt-acl.orig/src/driver.h +++ libvirt-acl/src/driver.h @@ -1098,6 +1098,14 @@ typedef char * (*virDrvNWFilterGetXMLDesc) (virNWFilterPtr pool, unsigned int flags); +# include "conf/domain_conf.h" + +typedef int + (*virDrvNWFilterInstantiateFilter) (virConnectPtr conn, + const virDomainNetDefPtr net); + +typedef void + (*virDrvNWFilterTeardownFilter) (virDomainNetDefPtr net); typedef struct _virNWFilterDriver virNWFilterDriver; typedef virNWFilterDriver *virNWFilterDriverPtr; @@ -1124,6 +1132,8 @@ struct _virNWFilterDriver { virDrvNWFilterDefineXML defineXML; virDrvNWFilterUndefine undefine; virDrvNWFilterGetXMLDesc getXMLDesc; + virDrvNWFilterInstantiateFilter privateInstantiateFilter; + virDrvNWFilterTeardownFilter privateTeardownFilter; };
On Mon, Jun 14, 2010 at 07:49:50AM -0400, Stefan Berger wrote:
Hello!
I am trying to fix the loadable modules support by removing all direct calls from qemu_driver.c and qemu_conf.c into nwfilter functions. The below patch extends the nwfilter driver interface with 2 more private functions to instantiate and tear down the filters. I call them 'private' because no client should ever be calling them via RPC and be able to tear down the filters for example -- they should only ever be callable from within libvirtd.
I extended the qemudShutdownVMDaemon function with the virConnectPtr type of parameter so that I have the pointer to conn->nwfilterDriver to access the private nwfilter driver functions. The problem with that is that the conn pointer is not always available to be passed (qemuReconnectDomain, qemuHandleMonitorEOF), particularly in tear-down functions, and so I have to pass a NULL pointer, which then prevents the calling of the private interface function to tear down the filters.Does anyone have a suggestion on how I would best change the interface with the nwfilter driver to fix this particular problem?
This problem is an artifact of trying to hook this into the main API driver tables. In our architecture we want the nwfilter & qemu drivers to be separately loadable modules, without any hard dependancy between them in either direction. Thus the first step is to kill the following two includes from the QEMU driver source, since using any functions from this header implies that there's a hard compile time dependancy from QEMU to nwfilter diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a4f06fb..6d99044 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -54,7 +54,6 @@ #include "network.h" #include "macvtap.h" #include "cpu/cpu.h" -#include "nwfilter/nwfilter_gentech_driver.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 670ef5d..f9deff6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -81,7 +81,6 @@ #include "xml.h" #include "cpu/cpu.h" #include "macvtap.h" -#include "nwfilter/nwfilter_gentech_driver.h" #include "hooks.h" #include "storage_file.h" The resulting compile errors show: qemu/qemu_conf.c: In function ‘qemudNetworkIfaceConnect’: qemu/qemu_conf.c:1694: error: implicit declaration of function ‘virNWFilterInstantiateFilter’ [-Wimplicit-function-declaration] qemu/qemu_conf.c:1694: error: nested extern declaration of ‘virNWFilterInstantiateFilter’ [-Wnested-externs] qemu/qemu_conf.c: In function ‘qemudBuildCommandLine’: qemu/qemu_conf.c:4213: error: implicit declaration of function ‘virNWFilterTearNWFilter’ [-Wimplicit-function-declaration] qemu/qemu_conf.c:4213: error: nested extern declaration of ‘virNWFilterTearNWFilter’ [-Wnested-externs] qemu/qemu_driver.c: In function ‘qemudStartVMDaemon’: qemu/qemu_driver.c:3566: error: implicit declaration of function ‘virNWFilterTearVMNWFilters’ [-Wimplicit-function-declaration] qemu/qemu_driver.c:3566: error: nested extern declaration of ‘virNWFilterTearVMNWFilters’ [-Wnested-externs] qemu/qemu_driver.c: In function ‘qemudDomainAttachNetDevice’: qemu/qemu_driver.c:7634: error: implicit declaration of function ‘virNWFilterTearNWFilter’ [-Wimplicit-function-declaration] qemu/qemu_driver.c:7634: error: nested extern declaration of ‘virNWFilterTearNWFilter’ [-Wnested-externs] For loadable modules to work, we need to be able to call those functions regardless of whether any module is loaded. I would thus suggest we introduce two new files: src/conf/domain_nwfilter.h src/conf/domain_nwfilter.c And add them to DOMAIN_CONF_SOURCES in Makefile.am so that they are compiled into the main binary at all times, thus accessible to all driver loadable modules. They should not contain any functional code, just be a glue layer similar to driver.h, but without any virConnectPtr involved: typedef int (*virDomainConfNetFilterSetupDrv)(virDomainNetDefPtr net); typedef int (*virDomainConfNetFilterCleanupDrv)(virDomainNetDefPtr net); typedef struct { virDomainConfNetFilterSetupDrv net; virDomainConfNetFilterCleanupDrv net; } virDomainConfNetFilterDrv; void virDomainConfNetFilterRegister(virDomainConfNetFilterDrv driver); int virDomainConfNetFilterSetup(virDomainNetDefPtr net); int virDomainConfNetFilterCleanup(virDomainNetDefPtr net); So, when the nwfilter driver is loaded it calls virDomainConfNetFilterRegister() to register the setup/teardown functions. When QEMU needs to add/remove a NIC it calls virDomainConfNetFilterSetup and virDomainConfNetFilterCleanup appropriately. None of the code now requires any virConnectPtr object, and we have totally decoupled the QEMU and NWfilter drivers Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
Stefan Berger