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([i], [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,
- );
-
-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,
- ) < 0)
-ret = -1;
-}
-
-if (ret < 0) {
-cb.step = STEP_TEAR_NEW; /* rollback */
-
-for (i = 0; i < nCallbackDriver; i++)
-callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB,
- );
-} else {
-cb.step = STEP_TEAR_OLD; /* switch over */
-
-for (i = 0; i < nCallbackDriver; i++)
-callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB,
- );
-}
-
-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();
initialized = false;
-virNWFilterDomainFWUpdateOpaque = NULL;
-virNWFilterDomainFWUpdateCB = NULL;
+rebuildCallback = NULL;
+rebuildOpaque
