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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
