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

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(a)redhat.com&gt; >>> --- >>> 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?