Re: [libvirt] [PATCH v3 17/20] nwfilter: remove virt driver callback layer for rebuilding filters
On Mon, Jun 18, 2018 at 04:59:37PM -0400, John Ferlan wrote: > > > On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote: > > Now that the nwfilter driver keeps a list of bindings that it has > > created, there is no need for the complex virt driver callbacks. It is > > possible to simply iterate of the list of recorded filter bindings. > > > > This means that rebuilding filters no longer has to acquire any locks on > > the virDomainObj objects, as they're never touched. > > > > Signed-off-by: Daniel P. Berrangé > > --- > > src/conf/nwfilter_conf.c | 134 +++- > > src/conf/nwfilter_conf.h | 51 +--- > > src/conf/virnwfilterobj.c | 4 +- > > src/libvirt_private.syms | 7 +- > > src/lxc/lxc_driver.c | 28 - > > src/nwfilter/nwfilter_driver.c | 21 ++-- > > src/nwfilter/nwfilter_gentech_driver.c | 167 - > > src/nwfilter/nwfilter_gentech_driver.h | 4 +- > > src/qemu/qemu_driver.c | 25 > > src/uml/uml_driver.c | 29 - > > 10 files changed, 141 insertions(+), 329 deletions(-) > > > > A diffstat that Jano usually applauds! Miracles do happen when you > close your eyes and say 3 times "I wish this code would just go away" > ;-). Still this is some of the most "entertaining code" - that now used > to exist! It seems I can dig up my nwfilter obj/hash code once this is > in... > > There's a couple nits below... > > Reviewed-by: John Ferlan > > John > > > > diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c > > index de26a6d034..5bb8a0c2e7 100644 > > --- a/src/conf/nwfilter_conf.c > > +++ b/src/conf/nwfilter_conf.c > > [...] > > > + > > + > > +int > > +virNWFilterTriggerRebuild(void) > > +{ > > +return rebuildCallback(rebuildOpaque); > > In the better safe than sorry - should we gate on "if > (rebuildCallback)"? I don't think there's a way into here with it being > NULL, but those are always "famous last words". Yeah ok. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 17/20] nwfilter: remove virt driver callback layer for rebuilding filters
On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote: > Now that the nwfilter driver keeps a list of bindings that it has > created, there is no need for the complex virt driver callbacks. It is > possible to simply iterate of the list of recorded filter bindings. > > This means that rebuilding filters no longer has to acquire any locks on > the virDomainObj objects, as they're never touched. > > Signed-off-by: Daniel P. Berrangé > --- > src/conf/nwfilter_conf.c | 134 +++- > src/conf/nwfilter_conf.h | 51 +--- > src/conf/virnwfilterobj.c | 4 +- > src/libvirt_private.syms | 7 +- > src/lxc/lxc_driver.c | 28 - > src/nwfilter/nwfilter_driver.c | 21 ++-- > src/nwfilter/nwfilter_gentech_driver.c | 167 - > src/nwfilter/nwfilter_gentech_driver.h | 4 +- > src/qemu/qemu_driver.c | 25 > src/uml/uml_driver.c | 29 - > 10 files changed, 141 insertions(+), 329 deletions(-) > A diffstat that Jano usually applauds! Miracles do happen when you close your eyes and say 3 times "I wish this code would just go away" ;-). Still this is some of the most "entertaining code" - that now used to exist! It seems I can dig up my nwfilter obj/hash code once this is in... There's a couple nits below... Reviewed-by: John Ferlan John > diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c > index de26a6d034..5bb8a0c2e7 100644 > --- a/src/conf/nwfilter_conf.c > +++ b/src/conf/nwfilter_conf.c [...] > + > + > +int > +virNWFilterTriggerRebuild(void) > +{ > +return rebuildCallback(rebuildOpaque); In the better safe than sorry - should we gate on "if (rebuildCallback)"? I don't think there's a way into here with it being NULL, but those are always "famous last words". > } > > [...] > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c > index 2388e925fc..3b111e3dc7 100644 > --- a/src/nwfilter/nwfilter_driver.c > +++ b/src/nwfilter/nwfilter_driver.c > @@ -163,6 +163,14 @@ nwfilterDriverInstallDBusMatches(DBusConnection *sysbus > ATTRIBUTE_UNUSED) > > #endif /* HAVE_FIREWALLD */ > > +static int virNWFilterTriggerRebuildImpl(void *opaque) NIT: static int virNWFilterTriggerRebuildImpl(void *opaque) > +{ > +virNWFilterDriverStatePtr nwdriver = opaque; > + > +return virNWFilterBuildAll(nwdriver, true); > +} > + > + [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 17/20] nwfilter: remove virt driver callback layer for rebuilding filters
Now that the nwfilter driver keeps a list of bindings that it has created, there is no need for the complex virt driver callbacks. It is possible to simply iterate of the list of recorded filter bindings. This means that rebuilding filters no longer has to acquire any locks on the virDomainObj objects, as they're never touched. Signed-off-by: Daniel P. Berrangé --- src/conf/nwfilter_conf.c | 134 +++- src/conf/nwfilter_conf.h | 51 +--- src/conf/virnwfilterobj.c | 4 +- src/libvirt_private.syms | 7 +- src/lxc/lxc_driver.c | 28 - src/nwfilter/nwfilter_driver.c | 21 ++-- src/nwfilter/nwfilter_gentech_driver.c | 167 - src/nwfilter/nwfilter_gentech_driver.h | 4 +- src/qemu/qemu_driver.c | 25 src/uml/uml_driver.c | 29 - 10 files changed, 141 insertions(+), 329 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index de26a6d034..5bb8a0c2e7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2819,121 +2819,6 @@ virNWFilterSaveConfig(const char *configDir, } -int nCallbackDriver; -#define MAX_CALLBACK_DRIVER 10 -static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; - -void -virNWFilterRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd) -{ -if (nCallbackDriver < MAX_CALLBACK_DRIVER) -callbackDrvArray[nCallbackDriver++] = cbd; -} - - -void -virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd) -{ -size_t i = 0; - -while (i < nCallbackDriver && callbackDrvArray[i] != cbd) -i++; - -if (i < nCallbackDriver) { -memmove(&callbackDrvArray[i], &callbackDrvArray[i+1], -(nCallbackDriver - i - 1) * sizeof(callbackDrvArray[i])); -callbackDrvArray[i] = 0; -nCallbackDriver--; -} -} - - -void -virNWFilterCallbackDriversLock(void) -{ -size_t i; - -for (i = 0; i < nCallbackDriver; i++) -callbackDrvArray[i]->vmDriverLock(); -} - - -void -virNWFilterCallbackDriversUnlock(void) -{ -size_t i; - -for (i = 0; i < nCallbackDriver; i++) -callbackDrvArray[i]->vmDriverUnlock(); -} - - -static virDomainObjListIterator virNWFilterDomainFWUpdateCB; -static void *virNWFilterDomainFWUpdateOpaque; - -/** - * virNWFilterInstFiltersOnAllVMs: - * Apply all filters on all running VMs. Don't terminate in case of an - * error. This should be called upon reloading of the driver. - */ -int -virNWFilterInstFiltersOnAllVMs(void) -{ -size_t i; -struct domUpdateCBStruct cb = { -.opaque = virNWFilterDomainFWUpdateOpaque, -.step = STEP_APPLY_CURRENT, -.skipInterfaces = NULL, /* not needed */ -}; - -for (i = 0; i < nCallbackDriver; i++) -callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); - -return 0; -} - - -int -virNWFilterTriggerVMFilterRebuild(void) -{ -size_t i; -int ret = 0; -struct domUpdateCBStruct cb = { -.opaque = virNWFilterDomainFWUpdateOpaque, -.step = STEP_APPLY_NEW, -.skipInterfaces = virHashCreate(0, NULL), -}; - -if (!cb.skipInterfaces) -return -1; - -for (i = 0; i < nCallbackDriver; i++) { -if (callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb) < 0) -ret = -1; -} - -if (ret < 0) { -cb.step = STEP_TEAR_NEW; /* rollback */ - -for (i = 0; i < nCallbackDriver; i++) -callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); -} else { -cb.step = STEP_TEAR_OLD; /* switch over */ - -for (i = 0; i < nCallbackDriver; i++) -callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); -} - -virHashFree(cb.skipInterfaces); - -return ret; -} - - int virNWFilterDeleteDef(const char *configDir, virNWFilterDefPtr def) @@ -3204,16 +3089,18 @@ virNWFilterDefFormat(const virNWFilterDef *def) return NULL; } +static virNWFilterTriggerRebuildCallback rebuildCallback; +static void *rebuildOpaque; int -virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, +virNWFilterConfLayerInit(virNWFilterTriggerRebuildCallback cb, void *opaque) { if (initialized) return -1; -virNWFilterDomainFWUpdateCB = domUpdateCB; -virNWFilterDomainFWUpdateOpaque = opaque; +rebuildCallback = cb; +rebuildOpaque = opaque; initialized = true; @@ -3233,8 +3120,15 @@ virNWFilterConfLayerShutdown(void) virRWLockDestroy(&updateLock); initialized = false; -virNWFilterDomainFWUpdateOpaque = NULL; -virNWFilterDomainFWUpdateCB