Re: [libvirt] [PATCH v3 16/20] nwfilter: keep track of active filter bindings
On Mon, Jun 18, 2018 at 04:59:12PM -0400, John Ferlan wrote:
>
>
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Currently the nwfilter driver does not keep any record of what filter
> > bindings it has active. This means that when it needs to recreate
> > filters, it has to rely on triggering callbacks provided by the virt
> > drivers. This introduces a hash table recording the virNWFilterBinding
> > objects so the driver has a record of all active filters.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > src/conf/virnwfilterobj.h | 4 ++
> > src/nwfilter/nwfilter_driver.c | 86 --
> > 2 files changed, 65 insertions(+), 25 deletions(-)
> >
>
> [...]
>
> > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> > index 7202691646..2388e925fc 100644
> > --- a/src/nwfilter/nwfilter_driver.c
> > +++ b/src/nwfilter/nwfilter_driver.c
> > @@ -38,7 +38,6 @@
> > #include "domain_conf.h"
> > #include "domain_nwfilter.h"
> > #include "nwfilter_driver.h"
> > -#include "virnwfilterbindingdef.h"
> > #include "nwfilter_gentech_driver.h"
> > #include "configmake.h"
> > #include "virfile.h"
> > @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged,
> > virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> > void *opaque ATTRIBUTE_UNUSED)
> > {
>
> [...]
>
> >
> > if (virNWFilterObjListLoadAllConfigs(driver->nwfilters,
> > driver->configDir) < 0)
> > goto error;
> >
> > +if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings,
> > driver->bindingDir) < 0)
> > +goto error;
> > +
>
> Because of this... [1]
>
> > nwfilterDriverUnlock();
> >
> > return 0;
> >
>
> [...]
>
> > @@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname,
> >const unsigned char *vmuuid,
> >virDomainNetDefPtr net)
> > {
> > -virNWFilterBindingDefPtr binding;
> > +virNWFilterBindingObjPtr obj;
> > +virNWFilterBindingDefPtr def;
> > int ret;
> >
> > -if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> > +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
> > net->ifname);
> > +if (obj) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Filter already present for NIC %s"),
> > net->ifname);
>
> [1]
> If I stop at this patch, start a domain with a filter, then restart
> libvirtd, then that will cause a guest running with a filter to exit and
> not just disappear - as it can be restarted. The error is:
>
> 2018-06-18 16:49:07.405+: 3978: error :
> nwfilterInstantiateFilter:666 : internal error: Filter already present
> for NIC vnet1
>
> Because once I have this patch built/running the
> /var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when
> virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize.
>
> I only saw this because I found later in patch 20 the failure comes from
> nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate
> is called. I worked my way back to this point.
>
> Not sure which would be the "best" solution at this point. Should we
> wait to do [1] until patch 20 or perhaps just not have an error here.
The current semantics are that nwfilterInstantiateFilter will not
report an error if the filter already exists, so I think I'll just
not report an error here. This method will go away anyway, so no
great loss.
>
> NB: If the guest was running at a point up through patch 15 then it
> won't exit on the first libvirtd restart (since the obj dir doesn't
> exist), but the issue shows up on the 2nd restart.
>
> In general the code is fine to me, but just need to handle this one
> issue in some way.
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 16/20] nwfilter: keep track of active filter bindings
On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> Currently the nwfilter driver does not keep any record of what filter
> bindings it has active. This means that when it needs to recreate
> filters, it has to rely on triggering callbacks provided by the virt
> drivers. This introduces a hash table recording the virNWFilterBinding
> objects so the driver has a record of all active filters.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> src/conf/virnwfilterobj.h | 4 ++
> src/nwfilter/nwfilter_driver.c | 86 --
> 2 files changed, 65 insertions(+), 25 deletions(-)
>
[...]
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 7202691646..2388e925fc 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -38,7 +38,6 @@
> #include "domain_conf.h"
> #include "domain_nwfilter.h"
> #include "nwfilter_driver.h"
> -#include "virnwfilterbindingdef.h"
> #include "nwfilter_gentech_driver.h"
> #include "configmake.h"
> #include "virfile.h"
> @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged,
> virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> void *opaque ATTRIBUTE_UNUSED)
> {
[...]
>
> if (virNWFilterObjListLoadAllConfigs(driver->nwfilters,
> driver->configDir) < 0)
> goto error;
>
> +if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings,
> driver->bindingDir) < 0)
> +goto error;
> +
Because of this... [1]
> nwfilterDriverUnlock();
>
> return 0;
>
[...]
> @@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname,
>const unsigned char *vmuuid,
>virDomainNetDefPtr net)
> {
> -virNWFilterBindingDefPtr binding;
> +virNWFilterBindingObjPtr obj;
> +virNWFilterBindingDefPtr def;
> int ret;
>
> -if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
> net->ifname);
> +if (obj) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Filter already present for NIC %s"), net->ifname);
[1]
If I stop at this patch, start a domain with a filter, then restart
libvirtd, then that will cause a guest running with a filter to exit and
not just disappear - as it can be restarted. The error is:
2018-06-18 16:49:07.405+: 3978: error :
nwfilterInstantiateFilter:666 : internal error: Filter already present
for NIC vnet1
Because once I have this patch built/running the
/var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when
virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize.
I only saw this because I found later in patch 20 the failure comes from
nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate
is called. I worked my way back to this point.
Not sure which would be the "best" solution at this point. Should we
wait to do [1] until patch 20 or perhaps just not have an error here.
NB: If the guest was running at a point up through patch 15 then it
won't exit on the first libvirtd restart (since the obj dir doesn't
exist), but the issue shows up on the 2nd restart.
In general the code is fine to me, but just need to handle this one
issue in some way.
John
> +virNWFilterBindingObjEndAPI();
> +return -1;
> +}
> +
> +if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> return -1;
> -ret = virNWFilterInstantiateFilter(driver, binding);
> -virNWFilterBindingDefFree(binding);
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 16/20] nwfilter: keep track of active filter bindings
Currently the nwfilter driver does not keep any record of what filter
bindings it has active. This means that when it needs to recreate
filters, it has to rely on triggering callbacks provided by the virt
drivers. This introduces a hash table recording the virNWFilterBinding
objects so the driver has a record of all active filters.
Signed-off-by: Daniel P. Berrangé
---
src/conf/virnwfilterobj.h | 4 ++
src/nwfilter/nwfilter_driver.c | 86 --
2 files changed, 65 insertions(+), 25 deletions(-)
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
index 433b0402d0..4a54dd50da 100644
--- a/src/conf/virnwfilterobj.h
+++ b/src/conf/virnwfilterobj.h
@@ -22,6 +22,7 @@
# include "internal.h"
# include "nwfilter_conf.h"
+# include "virnwfilterbindingobjlist.h"
typedef struct _virNWFilterObj virNWFilterObj;
typedef virNWFilterObj *virNWFilterObjPtr;
@@ -37,7 +38,10 @@ struct _virNWFilterDriverState {
virNWFilterObjListPtr nwfilters;
+virNWFilterBindingObjListPtr bindings;
+
char *configDir;
+char *bindingDir;
};
virNWFilterDefPtr
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 7202691646..2388e925fc 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -38,7 +38,6 @@
#include "domain_conf.h"
#include "domain_nwfilter.h"
#include "nwfilter_driver.h"
-#include "virnwfilterbindingdef.h"
#include "nwfilter_gentech_driver.h"
#include "configmake.h"
#include "virfile.h"
@@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged,
virStateInhibitCallback callback ATTRIBUTE_UNUSED,
void *opaque ATTRIBUTE_UNUSED)
{
-char *base = NULL;
DBusConnection *sysbus = NULL;
if (virDBusHasSystemBus() &&
@@ -191,6 +189,9 @@ nwfilterStateInitialize(bool privileged,
if (!(driver->nwfilters = virNWFilterObjListNew()))
goto error;
+if (!(driver->bindings = virNWFilterBindingObjListNew()))
+goto error;
+
if (!privileged)
return 0;
@@ -230,30 +231,35 @@ nwfilterStateInitialize(bool privileged,
goto error;
}
-if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0)
+if (VIR_STRDUP(driver->configDir, SYSCONFDIR "/libvirt/nwfilter") < 0)
goto error;
-if (virAsprintf(>configDir,
-"%s/nwfilter", base) == -1)
+if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) {
+virReportSystemError(errno, _("cannot create config directory '%s'"),
+ driver->configDir);
goto error;
+}
-VIR_FREE(base);
+if (VIR_STRDUP(driver->bindingDir, LOCALSTATEDIR
"/run/libvirt/nwfilter-binding") < 0)
+goto error;
-if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) {
+if (virFileMakePathWithMode(driver->bindingDir, S_IRWXU) < 0) {
virReportSystemError(errno, _("cannot create config directory '%s'"),
- driver->configDir);
+ driver->bindingDir);
goto error;
}
if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir)
< 0)
goto error;
+if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings,
driver->bindingDir) < 0)
+goto error;
+
nwfilterDriverUnlock();
return 0;
error:
-VIR_FREE(base);
nwfilterDriverUnlock();
nwfilterStateCleanup();
@@ -333,9 +339,12 @@ nwfilterStateCleanup(void)
nwfilterDriverRemoveDBusMatches();
VIR_FREE(driver->configDir);
+VIR_FREE(driver->bindingDir);
nwfilterDriverUnlock();
}
+virObjectUnref(driver->bindings);
+
/* free inactive nwfilters */
virNWFilterObjListFree(driver->nwfilters);
@@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname,
const unsigned char *vmuuid,
virDomainNetDefPtr net)
{
-virNWFilterBindingDefPtr binding;
+virNWFilterBindingObjPtr obj;
+virNWFilterBindingDefPtr def;
int ret;
-if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
+obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
net->ifname);
+if (obj) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Filter already present for NIC %s"), net->ifname);
+virNWFilterBindingObjEndAPI();
+return -1;
+}
+
+if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
return -1;
-ret = virNWFilterInstantiateFilter(driver, binding);
-virNWFilterBindingDefFree(binding);
+
+obj = virNWFilterBindingObjListAdd(driver->bindings,
+ def);
+if (!obj) {
+virNWFilterBindingDefFree(def);
+return -1;
+}
+
+ret = virNWFilterInstantiateFilter(driver, def);
+
+if (ret >= 0)
+
