src/conf/domain_nwfilter.c | 15 +++------------ src/conf/domain_nwfilter.h | 3 +-- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_interface.c | 4 ++-- src/qemu/qemu_process.c | 23 +++++++++++++++-------- src/uml/uml_conf.c | 2 +- 7 files changed, 25 insertions(+), 28 deletions(-)
https://bugzilla.redhat.com/show_bug.cgi?id=1607202
It's essentially stated in the nwfilterBindingDelete that we
will allow the admin to shoot themselves in the foot by deleting
the nwfilter binding which then allows them to undefine the
nwfilter that is in use for the running guest...
However, by allowing this we cause a problem for libvirtd
restart reconnect processing which would then try to recreate
the missing binding attempting to use the deleted filter
resulting in an error and thus shutting the guest down.
So rather than keep adding virDomainConfNWFilterInstantiate
flags to "ignore" specific error conditions and since (so far)
this is the only path that cared about checking if the filter
already exists and ignoring, let's just ignore all errors and
make the qemuProcessFiltersInstantiate be a void which will
attempt to check all networks for bindings and reload all filters
that exist. Using the VIR_INFO in order to at least "log" the
avoided issue.
This also means virDomainConfNWFilterInstantiate no longer
needs to handle/check the ignoreExists possbility.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
v1: https://www.redhat.com/archives/libvir-list/2018-August/msg01407.html
Changes - removed the ignoreExists and just change the logic for
reconnect processing to essentially ignore all errors. If it's felt
the VIR_INFO would be too chatty (especially since it'll be generated
for every already defined filter binding), I can remove it. Another
option would be to keep the ignoreExists logic and only generate that
VIR_INFO for "other" messages. In that case, I'd probably want to change
it to a VIR_WARN. Still figured I'd post the remove it all option first
for consideration with this caveat so that "option" can be considered
as well.
src/conf/domain_nwfilter.c | 15 +++------------
src/conf/domain_nwfilter.h | 3 +--
src/lxc/lxc_process.c | 2 +-
src/qemu/qemu_hotplug.c | 4 ++--
src/qemu/qemu_interface.c | 4 ++--
src/qemu/qemu_process.c | 23 +++++++++++++++--------
src/uml/uml_conf.c | 2 +-
7 files changed, 25 insertions(+), 28 deletions(-)
diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
index f39c8a1f9b..51c9063ca7 100644
--- a/src/conf/domain_nwfilter.c
+++ b/src/conf/domain_nwfilter.c
@@ -84,8 +84,7 @@ virNWFilterBindingDefForNet(const char *vmname,
int
virDomainConfNWFilterInstantiate(const char *vmname,
const unsigned char *vmuuid,
- virDomainNetDefPtr net,
- bool ignoreExists)
+ virDomainNetDefPtr net)
{
virConnectPtr conn = virGetConnectNWFilter();
virNWFilterBindingDefPtr def = NULL;
@@ -93,20 +92,12 @@ virDomainConfNWFilterInstantiate(const char *vmname,
char *xml = NULL;
int ret = -1;
- VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d",
- vmname, NULLSTR(net->ifname), NULLSTR(net->filter), ignoreExists);
+ VIR_DEBUG("vmname=%s portdev=%s filter=%s",
+ vmname, NULLSTR(net->ifname), NULLSTR(net->filter));
if (!conn)
goto cleanup;
- if (ignoreExists) {
- binding = virNWFilterBindingLookupByPortDev(conn, net->ifname);
- if (binding) {
- ret = 0;
- goto cleanup;
- }
- }
-
if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
goto cleanup;
diff --git a/src/conf/domain_nwfilter.h b/src/conf/domain_nwfilter.h
index 6bda228fc8..d2ebeff853 100644
--- a/src/conf/domain_nwfilter.h
+++ b/src/conf/domain_nwfilter.h
@@ -25,8 +25,7 @@
int virDomainConfNWFilterInstantiate(const char *vmname,
const unsigned char *vmuuid,
- virDomainNetDefPtr net,
- bool ignoreExists);
+ virDomainNetDefPtr net);
void virDomainConfNWFilterTeardown(virDomainNetDefPtr net);
void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm);
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 33c806630b..86f7463e53 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -303,7 +303,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
}
if (net->filter &&
- virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0)
+ virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net) < 0)
goto cleanup;
ret = containerVeth;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0b84a503bb..38c74bd9b1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3435,7 +3435,7 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm,
if (newdev->filter &&
virDomainConfNWFilterInstantiate(vm->def->name,
- vm->def->uuid, newdev, false) < 0) {
+ vm->def->uuid, newdev) < 0) {
virErrorPtr errobj;
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -3444,7 +3444,7 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm,
olddev->ifname);
virErrorPreserveLast(&errobj);
ignore_value(virDomainConfNWFilterInstantiate(vm->def->name,
- vm->def->uuid, olddev, false));
+ vm->def->uuid, olddev));
virErrorRestore(&errobj);
return -1;
}
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index a3f13093f5..5d54a85c53 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -467,7 +467,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
goto cleanup;
if (net->filter &&
- virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) {
+ virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) {
goto cleanup;
}
@@ -586,7 +586,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
goto cleanup;
if (net->filter &&
- virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) {
+ virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) {
goto cleanup;
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ab749389ee..48d9bab128 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3160,22 +3160,30 @@ qemuProcessNotifyNets(virDomainDefPtr def)
}
}
-static int
-qemuProcessFiltersInstantiate(virDomainDefPtr def, bool ignoreExists)
+
+/* Attempt to instantiate the filters. Ignore failures because it's
+ * (primarily) possible that a filter binding either already exists
+ * or someone deleted it and the associated filter while the guest
+ * was running and we don't want that action to cause failure to
+ * keep the guest running during the reconnection processing. */
+static void
+qemuProcessFiltersInstantiate(virDomainDefPtr def)
{
size_t i;
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
if ((net->filter) && (net->ifname)) {
- if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net, ignoreExists) < 0)
- return 1;
+ if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) {
+ VIR_INFO("filter '%s' instantiation for '%s' failed '%s'",
+ net->filter, net->ifname, virGetLastErrorMessage());
+ virResetLastError();
+ }
}
}
-
- return 0;
}
+
static int
qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm)
{
@@ -7892,8 +7900,7 @@ qemuProcessReconnect(void *opaque)
qemuProcessNotifyNets(obj->def);
- if (qemuProcessFiltersInstantiate(obj->def, true))
- goto error;
+ qemuProcessFiltersInstantiate(obj->def);
if (qemuProcessRefreshDisks(driver, obj, QEMU_ASYNC_JOB_NONE) < 0)
goto error;
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index f116e619ef..9c548f0e80 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -137,7 +137,7 @@ umlConnectTapDevice(virDomainDefPtr vm,
}
if (net->filter) {
- if (virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0) {
+ if (virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net) < 0) {
if (template_ifname)
VIR_FREE(net->ifname);
goto error;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote:
> +static void
> +qemuProcessFiltersInstantiate(virDomainDefPtr def)
> {
> size_t i;
>
> for (i = 0; i < def->nnets; i++) {
> virDomainNetDefPtr net = def->nets[i];
> if ((net->filter) && (net->ifname)) {
> - if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net, ignoreExists) < 0)
> - return 1;
> + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) {
> + VIR_INFO("filter '%s' instantiation for '%s' failed '%s'",
> + net->filter, net->ifname, virGetLastErrorMessage());
Won't this cause a log message on every single running guests on every
libvirtd restart, since the normal scenario is that the filter binding
exists ?
> + virResetLastError();
> + }
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
On 08/24/2018 09:04 AM, Daniel P. Berrangé wrote:
> On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote:
>> +static void
>> +qemuProcessFiltersInstantiate(virDomainDefPtr def)
>> {
>> size_t i;
>>
>> for (i = 0; i < def->nnets; i++) {
>> virDomainNetDefPtr net = def->nets[i];
>> if ((net->filter) && (net->ifname)) {
>> - if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net, ignoreExists) < 0)
>> - return 1;
>> + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) {
>> + VIR_INFO("filter '%s' instantiation for '%s' failed '%s'",
>> + net->filter, net->ifname, virGetLastErrorMessage());
>
> Won't this cause a log message on every single running guests on every
> libvirtd restart, since the normal scenario is that the filter binding
> exists ?
>
Yes, that was explained under the ---
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Aug 24, 2018 at 09:05:06AM -0400, John Ferlan wrote:
>
>
> On 08/24/2018 09:04 AM, Daniel P. Berrangé wrote:
> > On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote:
> >> +static void
> >> +qemuProcessFiltersInstantiate(virDomainDefPtr def)
> >> {
> >> size_t i;
> >>
> >> for (i = 0; i < def->nnets; i++) {
> >> virDomainNetDefPtr net = def->nets[i];
> >> if ((net->filter) && (net->ifname)) {
> >> - if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net, ignoreExists) < 0)
> >> - return 1;
> >> + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) {
> >> + VIR_INFO("filter '%s' instantiation for '%s' failed '%s'",
> >> + net->filter, net->ifname, virGetLastErrorMessage());
> >
> > Won't this cause a log message on every single running guests on every
> > libvirtd restart, since the normal scenario is that the filter binding
> > exists ?
> >
>
> Yes, that was explained under the ---
Next time I will actually read the text :-)
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
On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1607202 > > It's essentially stated in the nwfilterBindingDelete that we > will allow the admin to shoot themselves in the foot by deleting > the nwfilter binding which then allows them to undefine the > nwfilter that is in use for the running guest... > > However, by allowing this we cause a problem for libvirtd > restart reconnect processing which would then try to recreate > the missing binding attempting to use the deleted filter > resulting in an error and thus shutting the guest down. > > So rather than keep adding virDomainConfNWFilterInstantiate > flags to "ignore" specific error conditions and since (so far) > this is the only path that cared about checking if the filter > already exists and ignoring, let's just ignore all errors and > make the qemuProcessFiltersInstantiate be a void which will > attempt to check all networks for bindings and reload all filters > that exist. Using the VIR_INFO in order to at least "log" the > avoided issue. > > This also means virDomainConfNWFilterInstantiate no longer > needs to handle/check the ignoreExists possbility. > > Signed-off-by: John Ferlan <jferlan@redhat.com> > --- > > v1: https://www.redhat.com/archives/libvir-list/2018-August/msg01407.html > > Changes - removed the ignoreExists and just change the logic for > reconnect processing to essentially ignore all errors. If it's felt > the VIR_INFO would be too chatty (especially since it'll be generated > for every already defined filter binding), I can remove it. Another > option would be to keep the ignoreExists logic and only generate that > VIR_INFO for "other" messages. In that case, I'd probably want to change > it to a VIR_WARN. Still figured I'd post the remove it all option first > for consideration with this caveat so that "option" can be considered > as well. I guess the difference with 'ignore exists' is that this situation is harmless. The binding is still active and thus network traffic is confined. In the other error scenarios, the failure indicates a problem that likely means the network traffic is not confined. So it probably makes sense to keep the ignoreExists flag and use VIR_WARN for other errors. 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
© 2016 - 2024 Red Hat, Inc.