[libvirt] [PATCH 03/17] nwfilter: Fix possible locking problem in LoadConfig error path

[John Ferlan]

On 07/14/2017 08:09 AM, Michal Privoznik wrote:
> On 07/14/2017 01:52 AM, John Ferlan wrote:
>>
>>
>> On 07/13/2017 10:41 AM, Michal Privoznik wrote:
>>> On 06/02/2017 12:25 PM, John Ferlan wrote:
>>>> After returning from virNWFilterObjListAssignDef the @obj is locked;
>>>> however, if virNWFilterSaveConfig fails to save the generated UUID
>>>> the code jumped to error and returns NULL meaning the caller will
>>>> not call virNWFilterObjUnlock on the object leaving the object
>>>> locked on list and ripe for a deadlock on any subsequent Find
>>>> of all objects or object removal.
>>>>
>>>> So rather than jumping to error alter the comment prior to the
>>>> virNWFilterSaveConfig and just provide a VIR_INFO message for anyone
>>>> that cares, but allow the code to continue and a subsequent LoadConfig
>>>> to once again attempt the save of a newly generated UUID.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>  src/conf/virnwfilterobj.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>>>> index 3fb6046..0343c0a 100644
>>>> --- a/src/conf/virnwfilterobj.c
>>>> +++ b/src/conf/virnwfilterobj.c
>>>> @@ -507,10 +507,13 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
>>>>      def = NULL;
>>>>      objdef = obj->def;
>>>>  
>>>> -    /* We generated a UUID, make it permanent by saving the config to disk */
>>>> +    /* We generated a UUID, atttempt to make it permanent by saving the
>>>
>>> s/ttt/tt/
>>>
>>>> +     * config to disk. If not successful, no need to fail or remove the
>>>> +     * object as a future load would regenerate a UUID and try again,
>>>> +     * but the existing config would still exist and can be used. */
>>>>      if (!objdef->uuid_specified &&
>>>>          virNWFilterSaveConfig(configDir, objdef) < 0)
>>>> -        goto error;
>>>> +        VIR_INFO("failed to save generated UUID for filter '%s'", objdef->name);
>>>>  
>>>>      VIR_FREE(configFile);
>>>>      return obj;
>>>>
>>>
>>> Well, frankly I'd say that we should not even try to save the config in
>>> the first place. Load() should really just load. We shouldn't try to
>>> "fix" XML configs at load time rather then when saving it in define phase.
>>>
>>
>> IIRC: this one's a bit weird since if the UUID doesn't exist we "can"
>> generate one. If we generate one, then we should save it. However,
>> failing to save shouldn't be called an error because having a UUID isn't
>> required it's just something we try to "enforce" at some point in time
>> of the nwfilter.  I kind of didn't want to "adjust" that logic. That's a
>> different patch ;->
> 
> Ah, so I've dug out the commit that introduced this behaviour: 441e881e.
> It's fairly recent and it indeed fixes a problem we have. Well, I still
> rather see it as a separate operation than during config load. It could
> run right after we load configs. But whatever.
> 
> So the commit says there are troubles if we generate new UUIDs each
> time. If that's the case I don't think that failing to save should be
> ignored. It would lead to the same problem that the commit tried to fix,
> wouldn't it?
> 
Ahhh, I see.  Interesting commit...  Then of course there's b3e71a8830
which is the actual self inflicted shot. I need to revert that one ASAP
(and do the same for the 3.3, 3.4, and 3.5 -maint trees because there's
an awful double free bug for @def).

John