Re: [libvirt] [PATCH] nwfilter: intantiate filters on loading driver

On 10/30/18 3:24 AM, Nikolay Shirokovskiy wrote: First off let me say you certainly find a lot of interesting bugs/issues that are complex and that's good. Often times I wonder how you trip across things or how to quantify what you've found. Some are simpler than others. This one on the surface would seem to be simple, but I keep wondering is there a downside. The nwfilter processing is kindly said rather strange, complex, and to a degree fragile. Thus when patches come along they are met with greater scrutiny. From just reading the commit message here I don't get a sense for the problem and why the solution fixes it. So I'm left to try and research myself and ask a lot of questions. BTW, some of the detail provided in this response is really useful as either part of a cover or after the --- in the single patch. I would think you'd "know" when you've done lots of research into a problem that providing reviews more than a mere hint would be useful! For nwfilter bindings, it's hard to imagine this is one of those - it just happened type events. There seems to be a very specific sequence that's missing from the commit description. Ah yes, the virNWFilterConfLayerInit sets up the context to call the virNWFilterTriggerRebuildImpl for virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef and no I see it doesn't directly call the virNWFilterBuildAll. Still, I don't see where the processing of a rebuild callback is different than prior to commit 3df907bfff - at least with respect to the two places which would call it. Which list? nwfilters? nwfilter-bindings? And similar logic was run was before commit 3df907bfff? This direct correlation is the part I'm missing. What follows is my understand of the before and after - some of the before is a bit of hand waving. Perhaps Daniel can chime in and "fix up" inaccuracies. Prior to commit 3df907bfff, the virNWFilterDomainFWUpdateCB was only run when the domain was active. IOW: The domain would need to preload the bindings in "hidden" locations such that the various vnetX (or whatever target dev for the <interface>) devices would load the whichever <filterref> was assocated. When libvirtd was restarted the processing was similar and the "magic" of reinstantiation was provided through virNWFilterRegisterCallbackDriver in (for example) qemuStateInitialize. That I believe is the part you describe in your commit as "Before using filters binding filters instantiation was done by hypervisors drivers initialization code (qemu was the only such hypervisor)." - although I see LXC and UML drivers changed as well, so I could be wrong with my assumption of what you meant. After that commit, rather than requiring qemuStateInitialize to register a callback driver which somehow magically loaded the filters for running guests, the nwfilter-bindings for running guests are loaded via (for example) /var/run/libvirt/vnet0.xml file processing during virNWFilterBindingObjListLoadAllConfigs (change in the previous commit 57f5621f46). So rather than the rebuild processing magically occurring in the background by some hypervisor driver performing the rebuild callback processing. Since virRegisterStateDriver (and friends) for nwfilter are run before qemu, IIUC that means the filter bindings would be loaded already. It's all a complicated dance. So in this after model for running guests it seems to me that the exact same processing occurs. Now if someone during libvirtd's stop period does something "outside the scope" of libvirt to change things that libvirt wouldn't otherwise be notified about, then all bets are off. Similar for other pieces of the code such as CPU's, Memory, Storage, etc.; however, for those there is a query at reinitialization time that can help reconcile differences due to perhaps missed events from qemu because libvirtd wasn't processing. Not sure there's something similar to query for nwfilter and bindings, but I assume there wouldn't have been any before either. and this is the "key point" you are trying to reconcile, true? Can you provided a concrete example showing your steps to help clear up things for me? Using just one filter is fine. What does the guest look like before whatever it is you do is done? What steps do you take? Then what does it look like afterwards? What would you expect? If you did the same/similar steps prior to the referenced commits what was the result? As an "aside", it's been noted somewhere admins should not be messing with nwfilter bindings. If someone "cleans rules" during a period when libvirtd is stopped, then what is "expected" to happen afterwards. Tks John