Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted
On 08/23/2018 07:27 AM, Daniel P. Berrangé wrote:
On Wed, Aug 22, 2018 at 05:43:21PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1607202
>
> It's stated that if the admin wants to shoot themselves in
> the foot by removing the nwfilter binding while the domain
So based on your explanation in the other reply, this message
is what was misleading me. s/nwfilter binding/nwfilter/
Actually perhaps it's more by "first removing the nwfilter binding and
subsequently undefining the nwfilter that is/was in use for the running
guest..."
If just the nwfilter binding was removed, then libvirtd restart would
have been fine because it would have recreated the nwfilter binding. Of
course that may not be expected either...
> is running we will certainly allow that. However, in doing
> so we also run the risk that a libvirtd restart will cause
> the domain to be shutdown, which isn't a good thing.
>
> So add another boolean to virDomainConfNWFilterInstantiate
> which allows us to recover somewhat gracefully in the event
> the virNWFilterBindingCreateXML fails when we come from
> qemuProcessReconnect and we determine that the filter has
> been deleted. It was there at some point (it had to be), but
> if it's missing, then we don't want to cause the guest to
> stop running, so issue a warning and continue on.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/domain_nwfilter.c | 33 ++++++++++++++++++++++++++++-----
> src/conf/domain_nwfilter.h | 3 ++-
> src/lxc/lxc_process.c | 3 ++-
> src/qemu/qemu_hotplug.c | 7 ++++---
> src/qemu/qemu_interface.c | 6 ++++--
> src/qemu/qemu_process.c | 10 +++++++---
> src/uml/uml_conf.c | 3 ++-
> 7 files changed, 49 insertions(+), 16 deletions(-)
[snip]
> static int
> -qemuProcessFiltersInstantiate(virDomainDefPtr def, bool ignoreExists)
> +qemuProcessFiltersInstantiate(virDomainDefPtr def,
> + bool ignoreExists,
> + bool ignoreDeleted)
> {
> 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)
> + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
> + ignoreExists,
> + ignoreDeleted) < 0)
> return 1;
> }
Rather than this extra "ignoreDeleted" arg, why can't we just do
if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
ignoreExists) < 0 &&
ignoreDeleted)
return 1;
This ensures that all things which can cause a nwfilter binding failure
on startup will be handled by avoiding tearing down the running guest.
Did you mean !ignoreDeleted? There's only one caller to
qemuProcessFiltersInstantiate which does:
if (qemuProcessFiltersInstantiate(obj->def, true))
goto error;
Of course what's the purpose of distinguishing between ignoreExists and
ignoreDeleted then? Essentially what you're indicating is we wouldn't
care about any error because virDomainConfNWFilterInstantiate wouldn't
be distinguishing between any error (because there's only one caller to
qemuProcessFiltersInstantiate).
I could change the last argument to virDomainConfNWFilterInstantiate to
be a flag instead of a bool so that if we have future errors we care to
ignore we don't keep adding bool arguments, but that's just a
implementation detail.
Again, I could go back and figure out a way for the *Delete path to
remove the live net->filter based on the path taken so that when we get
here we wouldn't find the net->filter and thus wouldn't call the
instantiate function. It's just a different approach, but I'm hesitant
mainly because then one wouldn't be able to use the *CreateXML API to
reinstate the filter. I'm not as clear about all the (wacko) use cases.
John