On Thu, Aug 23, 2018 at 07:59:41AM -0400, John Ferlan wrote: > > > 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@xxxxxxxxxx> > >> --- > >> 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 Heh, yes. > 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). Oh thats a good point - I forgot ignoreExists was for the same reason. > > 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. We've deal with 1 problem scenario (alredy existing binding) and now adding a second (missing filter). Will someone find a third scenario and then a fourth. The flags argument ends up effectively being a bitmask of lines where we ignore errors. I'm wondering is it *ever* valid to treat failure of this filter call as a fatal problem and teardown an already running VM ? My gut says no. So perhaps we remove the ignoreExists parameter too, and just make that one caller simply ignore the errors on restarts. 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
