[libvirt] [PATCH] Revert "nwfilter: Move save of config until after successful assign"

[John Ferlan]

On 07/17/2017 08:44 AM, Pavel Hrdina wrote:
> On Mon, Jul 17, 2017 at 07:39:28AM -0400, John Ferlan wrote:
>>
>>
>> On 07/17/2017 04:15 AM, Pavel Hrdina wrote:
>>> On Sat, Jul 15, 2017 at 02:40:06PM -0400, John Ferlan wrote:
>>>> This reverts commit b3e71a8830b2683ee88fa10cb048eabb99a446c0.
>>>>
>>>> As it turns out this ends up very badly as the @def could be Free'd
>>>> even though it's owned by @obj as a result of the AssignDef.
>>>
>>> I don't see a reason to revert it.  What do you mean that the @def can
>>> be freed?  The virNWFilterObjListAssignDef() doesn't free the @def that
>>> is passed to it, it only assign it to nwfilter object and returns it
>>> immediately.
>>>
>>> Pavel
>>>
>>
>> After ListAssignDef the @def is owned by @obj
>>
>> If the SaveConfig fails, we jump to error: which will free @def. Now we
>> have an @obj in the nwfilters->objs list which has obj->def entry using
>> that address.
> 
> Oh, right, it might happen while reloading libvirtd daemon.
> 

Fortunately to a degree it's an error path on saving a file and only if
needing to generate the UUID - so the impact should be low. It's the
improbable, but possible case. If they were available, the patch should
be backported to 3.3-maint, 3.4-maint, and 3.5-maint trees. I've never
created one of those before though...

> Sigh, I hate the nwfilter code, it's way to complex and ugly and has a
> lot of workaround, like this UUID generation and saving it back to the
> config file while loading the config file.
> 

Yeah - it's really ugly and has quite a few "gotchas" that should be
handled better. The recursive locks are especially nightmarish. Once my
other series is complete, having hash table lookups instead of list
traversal and comparison should make a few things easier. Doubt anyone
wants to profess too much knowledge of the nwfilters in order to take on
the effort to clean things up.

Tks -

John

> Anyway, we should fix it even though reloading of libvirtd probably
> don't work.
> 
> Reviewed-by: Pavel Hrdina <phrdina at redhat.com>
>